Skip to content

Make local registry address configurable#853

Merged
hardys merged 1 commit intoopenshift-metal3:masterfrom
honza:local-registry
Nov 20, 2019
Merged

Make local registry address configurable#853
hardys merged 1 commit intoopenshift-metal3:masterfrom
honza:local-registry

Conversation

@honza
Copy link
Copy Markdown
Member

@honza honza commented Nov 19, 2019

Or, in other words, let's not hardcode it anymore

Comment thread common.sh Outdated
fi

if env | grep -q "_LOCAL_IMAGE=" ; then
export LOCAL_REGISTRY_ADDRESS = ${LOCAL_REGISTRY_ADDRESS:-"192.168.111.1:5000"}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Extra spaces

Suggested change
export LOCAL_REGISTRY_ADDRESS = ${LOCAL_REGISTRY_ADDRESS:-"192.168.111.1:5000"}
export LOCAL_REGISTRY_ADDRESS=${LOCAL_REGISTRY_ADDRESS:-"192.168.111.1:5000"}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

Comment thread common.sh Outdated
fi

if env | grep -q "_LOCAL_IMAGE=" ; then
export LOCAL_REGISTRY_ADDRESS = ${LOCAL_REGISTRY_ADDRESS:-"192.168.111.1:5000"}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO LOCAL_REGISTRY_ADDRESS shouldn't be exported within condition.
Being exported in outer block would allow images' mirroring(pr #850) not rely on presence of *_LOCAL_IMAGE variables, wdyt?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah agreed, and I think the changes in 04_setup_ironic.sh will break without this unless *_LOCAL_IMAGE is specified

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

Comment thread 04_setup_ironic.sh Outdated
#export IRONIC_INSPECTOR_LOCAL_IMAGE=https://github.com/metal3-io/ironic-inspector-image
#export IRONIC_RHCOS_DOWNLOADER_LOCAL_IMAGE=https://github.com/openshift-metal3/rhcos-downloader
#export BAREMETAL_OPERATOR_LOCAL_IMAGE=192.168.111.1:5000/localimages/bmo:latest
#export BAREMETAL_OPERATOR_LOCAL_IMAGE=$LOCAL_REGISTRY_ADDRESS/localimages/bmo:latest
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is just an example comment showing how to set this in config_$USER.sh, where common.sh isn't necessarily souurced, so maybe we should leave this one as-is?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown

@hardys hardys left a comment

Choose a reason for hiding this comment

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

Small fix required, otherwise lgtm

Or, in other words, let's not hardcode it anymore
@hardys hardys added the CI check this PR with CI label Nov 20, 2019
@hardys hardys self-requested a review November 20, 2019 11:41
Copy link
Copy Markdown

@hardys hardys left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@metal3ci
Copy link
Copy Markdown

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1317/

@hardys hardys merged commit 834c0e8 into openshift-metal3:master Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI check this PR with CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants