-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
modules/installation-mirror-repository: Drop unnecessary 'export' #22008
Conversation
The export landed with the module in ced98c6 (osdocs-626 preparing for disconnected installation, 2019-08-26, openshift#16678). But these are local variables that we expand when calling the mirror command. The mirror command receives them from its command line arguments; we don't need export [1] to expose them in the mirror command's environment as well. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#export
If no need export the variables, then we may update the mirror command with the local variables instead of env variables in "Mirror the repository" step too.
hdyt? @wking |
The example you have is what we have now in master. How did you want me to change it? Here's an example showing local variables expanded by the shell before they are fed to a subcommand, in case that helps clarify: $ A=whatever
$ python -c 'import os, sys; print(sys.argv, [k for k in os.environ.keys() if "A" in k])' "${A}"
(['-c', 'whatever'], [..., 'PATH', ...]) The |
I mean, if we want to keep all variables for long-time usage, then better to set env variables permanently(not just export in the shell). And if we update it to use local variables in one-time shell, then we can just use the command with correct variables. Maybe no need to set local variables before it and then call it.
I meant:
Anyway, it's not a big deal for this part, i think users can understand either, so it is ok for me to merge the pr without any changes. |
Folks can do that. But I don't think we want to get into that level of detail in the docs. Folks who are interested in declaring these in their
Isn't that what I'm doing? I think declaring them on separate lines like we've been doing gives space for the oc adm -a "${LOCAL_SECRET_JSON}" release mirror \
--from="quay.io/${PRODUCT_REPO}/${RELEASE_NAME}:${OCP_RELEASE}" \
--to="${LOCAL_REGISTRY}/${LOCAL_REPOSITORY}" \
--to-release-image="${LOCAL_REGISTRY}/${LOCAL_REPOSITORY}:${OCP_RELEASE}" and then a section that walked through:
etc. But that sort of change seemed like a bigger pivot than I want to argue through, so I'm just focusing on the
I am agnostic about any backporting. "No backporting at all", "backporting through all the versions", and anything in between are all fine with me. |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The export landed with the module in ced98c6 (#16678). But these are local variables that we expand when calling the mirror command. The mirror command receives them from its command line arguments; we don't need
export
to expose them in the mirror command's environment as well./assign @kalexand-rh