Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for S3 bucket as source for anod #669

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nikokrock
Copy link
Contributor

Part of it/org/software-supply-chain/product-security/issues#24

Part of it/org/software-supply-chain/product-security/issues#24
@Nikokrock Nikokrock requested a review from enzbang January 18, 2024 11:12
@enzbang enzbang self-requested a review January 24, 2024 09:02
@enzbang
Copy link
Member

enzbang commented Jan 24, 2024

Note that there are some errors

src/e3/anod/checkout.py:24: error: Invalid type alias: expression is not a valid type  [valid-type]
src/e3/anod/checkout.py:24: error: Unsupported left operand type for | ("object")  [operator]
src/e3/anod/checkout.py:63: error: Variable "e3.anod.checkout.SupportedVCS" is not valid as a type  [valid-type]
src/e3/anod/checkout.py:63: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
src/e3/anod/checkout.py:93: error: Missing positional argument "value" in call to "assert_never"  [call-arg]
Found 4 errors in 1 file (checked 96 source files)

@@ -20,6 +21,8 @@
from collections.abc import Callable
from e3.mypy import assert_never

SupportedVCS = Literal["git"] | Literal["svn"] | Literal["external"] | Literal["s3"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must use typing.Union. | is not supported for type aliasing.

@@ -84,6 +87,8 @@ def update(
update = self.update_svn
elif vcs == "external":
update = self.update_external
elif vcs == "s3":
update = self.update_s3
else:
assert_never()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function takes one argument as parameter.

"""Update working dir using a S3 bucket as source.

:param url: s3 url to a s3 directory. s3://[profile@]bucket/suffix
:param revision: ignored
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why create an ignored parameter?

# url validation
m = re.match("s3://([a-zA-Z0-9_-]+@)?(.+)$", url)
if m is None:
raise e3.error.E3Error("invalid s3 url: {url}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing f"..."


# Extract profile from the url and perform basic
# url validation
m = re.match("s3://([a-zA-Z0-9_-]+@)?(.+)$", url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should we compile this regex?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants