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

update ingestion script to support multiple S3 buckets #154

Merged
merged 4 commits into from
Jun 21, 2023

Conversation

boegel
Copy link
Contributor

@boegel boegel commented Jun 21, 2023

No description provided.

if args.list_only:
for num, tarball in enumerate(tarballs):
print(f'{num}: {tarball}')
sys.exit(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was already a bit ugly, but doesn't work as intended anymore at all now, as it will exit before the second bucket is being handled. Easy fix would be to change it in continue, but maybe it should be done a bit cleaner with a proper if/else or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in aed065a

@bedroge
Copy link
Collaborator

bedroge commented Jun 21, 2023

Gave it a quick try, and the new code seems to work fine, except that there is a bug in the existing find_tarballs function, it doesn't like empty buckets:

    for object in s3.list_objects_v2(Bucket=bucket)['Contents']
KeyError: 'Contents'

@bedroge
Copy link
Collaborator

bedroge commented Jun 21, 2023

Gave it a quick try, and the new code seems to work fine, except that there is a bug in the existing find_tarballs function, it doesn't like empty buckets:

    for object in s3.list_objects_v2(Bucket=bucket)['Contents']
KeyError: 'Contents'

This can actually be easily fixed by using:

       object['Key'] for object in s3.list_objects_v2(Bucket=bucket).get('Contents', [])

@boegel
Copy link
Contributor Author

boegel commented Jun 21, 2023

Gave it a quick try, and the new code seems to work fine, except that there is a bug in the existing find_tarballs function, it doesn't like empty buckets

fixed in cc63131

@boegel
Copy link
Contributor Author

boegel commented Jun 21, 2023

Gave it a quick try, and the new code seems to work fine, except that there is a bug in the existing find_tarballs function, it doesn't like empty buckets:

    for object in s3.list_objects_v2(Bucket=bucket)['Contents']
KeyError: 'Contents'

This can actually be easily fixed by using:

       object['Key'] for object in s3.list_objects_v2(Bucket=bucket).get('Contents', [])

OK, see ddef0c7

Copy link
Collaborator

@bedroge bedroge left a comment

Choose a reason for hiding this comment

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

Lgtm. One failing CI test, but that's completely unrelated to the changes made in this PR, so I'm ignoring it.

@bedroge bedroge merged commit 1bc508a into EESSI:main Jun 21, 2023
22 of 23 checks passed
@boegel
Copy link
Contributor Author

boegel commented Jun 21, 2023

opened an issue for CI problem: #155

@boegel boegel deleted the ingestion_staging_buckets branch June 21, 2023 07:03
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.

2 participants