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

Prevent S3 information on non-S3 mirrors #27701

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

Conversation

josephsnyder
Copy link
Contributor

Switch from looking at the presence of the S3 information keys to
determine if the dictionary URL is used to instead look at the value
in the keys.

Add s3_endpoint_url as an additional key value for the S3 information.

See #27694 (comment)

@josephsnyder josephsnyder force-pushed the bug/stop_adding_s3_connection_info_all_mirrors branch from e88bd22 to 55d1fda Compare November 29, 2021 21:13
@alalazo alalazo added this to In progress in Spack v0.18.0 release via automation Dec 1, 2021
Spack v0.18.0 release automation moved this from In progress to Reviewer approved Dec 7, 2021
opadron
opadron previously approved these changes Dec 7, 2021
@scheibelp
Copy link
Member

Does this fix #27694? If so, you can add Fixes #27694 to the initial post in this thread and it will be automatically closed when this is merged.

@josephsnyder
Copy link
Contributor Author

@scheibelp, #27694 is a pull request and the reason it is linked is that there is a comment requesting this particular change. I separated them to make things easier.

@josephsnyder josephsnyder force-pushed the bug/stop_adding_s3_connection_info_all_mirrors branch from 55d1fda to 79fcaf1 Compare December 10, 2021 13:18
Spack v0.18.0 release automation moved this from Reviewer approved to Review in progress Dec 10, 2021
@josephsnyder josephsnyder force-pushed the bug/stop_adding_s3_connection_info_all_mirrors branch from 79fcaf1 to 93cdae3 Compare December 17, 2021 19:24
@josephsnyder
Copy link
Contributor Author

@opadron, I'd like to keep these moving. Do you have the time for a review on this and #27694?

opadron
opadron previously approved these changes Dec 21, 2021
Spack v0.18.0 release automation moved this from Review in progress to Reviewer approved Dec 21, 2021
@alalazo alalazo added this to In progress in Spack v0.17.1 Release via automation Dec 22, 2021
@alalazo

This comment has been minimized.

@josephsnyder
Copy link
Contributor Author

josephsnyder commented Dec 22, 2021

@alalazo, I don't see that happening on the develop branch (commit SHA: b7b6542)

(spack_env) snyder@irune-journal:~/Work/ECP/spack$ ./bin/spack mirror add  minio s3://sample --s3-endpoint-url http://10.50.57.119:9000 --s3-access-key-id 892W696SB0HMCPS2BDIA --s3-access-key-secret <Secret>
(spack_env) snyder@irune-journal:~/Work/ECP/spack$ ./bin/spack mirror add  miniolocal file:///tmp/mirror
(spack_env) snyder@irune-journal:~/Work/ECP/spack$ ./bin/spack mirror list
spack-public    https://mirror.spack.io
miniolocal      file:///tmp/mirror (fetch)
miniolocal      file:///tmp/mirror (push)
minio           s3://sample (fetch)
minio           s3://sample (push)

@alalazo
Copy link
Member

alalazo commented Dec 22, 2021

@josephsnyder Yeah, sorry for the noise. I could reproduce the error I posted reliably until I tried:

$ spack clean -a

and cleaning the cache made it disappear somehow. I am not sure what could have caused it in the first place though I have been moving across branches a lot...

@alalazo alalazo removed this from In progress in Spack v0.17.1 Release Dec 23, 2021
@alalazo alalazo added this to In progress in Spack v0.17.2 Release via automation Dec 23, 2021
@josephsnyder josephsnyder force-pushed the bug/stop_adding_s3_connection_info_all_mirrors branch from 93cdae3 to dd28db3 Compare January 3, 2022 16:01
Spack v0.18.0 release automation moved this from Reviewer approved to Review in progress Jan 3, 2022
Spack v0.17.2 Release automation moved this from In progress to Review in progress Jan 3, 2022
Switch from looking at the presence of the S3 information keys to
determine if the dictionary URL is used to instead look at the value
in the keys.

Add s3_endpoint_url as an additional key value for the S3 information.
@josephsnyder josephsnyder force-pushed the bug/stop_adding_s3_connection_info_all_mirrors branch from dd28db3 to 6cce365 Compare January 11, 2022 15:59
@haampie haampie added this to In progress in Spack v0.17.3 Release via automation Mar 22, 2022
@haampie haampie removed this from Review in progress in Spack v0.17.2 Release Mar 22, 2022
# On creation, assume connection data is set for both
if any(value for value in key_values if value in args):
# Check for value in each key, instead of presence of each key
if any(vars(args)[value] for value in key_values if value in args):
Copy link
Member

Choose a reason for hiding this comment

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

I seem to recall that args is now supposed to be a namespace object with attributes instead of a mapping with keys. (That would explain the use of vars, here).

I think that's fine, but something would need to be done about the default for args, since it doesn't make sense to default to an empty dict, anymore.

Maybe default to None and have some branch that handles None?

Copy link
Member

Choose a reason for hiding this comment

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

The code is only called from one place, and it supplies args. Maybe it should just make args a required argument?

Spack v0.17.3 Release automation moved this from In progress to Review in progress May 23, 2022
@becker33 becker33 removed this from Review in progress in Spack v0.18.0 release May 24, 2022
@becker33 becker33 added this to In progress in Spack v0.18.1 release via automation May 24, 2022
@alalazo alalazo removed this from Review in progress in Spack v0.17.3 Release Jul 4, 2022
@alalazo alalazo added this to In progress in Spack v0.19.0 release via automation Jul 4, 2022
@alalazo alalazo removed this from In progress in Spack v0.18.1 release Jul 4, 2022
@tgamblin tgamblin added this to the v0.20.0 milestone Nov 7, 2022
@tgamblin tgamblin removed this from In progress in Spack v0.19.0 release Nov 7, 2022
@alalazo alalazo assigned haampie and unassigned opadron Feb 20, 2023
@alalazo alalazo removed this from the v0.20.0 milestone May 3, 2023
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.

None yet

7 participants