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

[CLOUD-2911] maven settings script improvements #303

Merged
merged 1 commit into from Dec 13, 2018
Merged

[CLOUD-2911] maven settings script improvements #303

merged 1 commit into from Dec 13, 2018

Conversation

errantepiphany
Copy link
Contributor

[CLOUD-2911] maven repository settings improvements
https://issues.jboss.org/browse/CLOUD-2911

Signed-off-by: David Ward dward@redhat.com

Thanks for submitting your Pull Request!

Please make sure your PR meets following requirements:

  • Pull Request title is properly formatted: [CLOUD-XYA] Subject
  • Pull Request contains link to the JIRA issue
  • Pull Request contains description of the issue
  • Pull request does not include fixes for other issues than the main ticket
  • Attached commits represent unit of work and are properly formatted
  • You have read and agreed to the Developer Certificate of Origin (DCO) (see CONTRIBUTING.md)
  • Every commit contains Signed-off-by: Your Name <yourname@redhat.com> - use git commit -s

@errantepiphany errantepiphany changed the title [CLOUD-2911] maven repository settings improvements [CLOUD-2911] maven settings script improvements Oct 31, 2018
@errantepiphany errantepiphany dismissed rcernich’s stale review October 31, 2018 16:06

Dismissing review and asking for a new one. :)

rcernich
rcernich previously approved these changes Nov 2, 2018
Copy link
Contributor

@rcernich rcernich left a comment

Choose a reason for hiding this comment

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

LGTM

local var_name=$prefix"_"$2
echo ${!var_name:-$3}
fi
}
# insert settings for mirrors/repository managers into settings.xml if supplied
# internal function, use process_maven_settings_xml which applies all configuration
function add_maven_mirrors() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest:
instead of 205-207:
local mirror_id=$(_maven_find_env "${maven_mirror_prefix}_MAVEN_MIRROR_ID" "mirror${counter}")
local mirror_url=$(_maven_find_env "${maven_mirror_prefix}_MAVEN_MIRROR_URL")
local mirror_of=$(_maven_find_env "${maven_mirror_prefix}_MAVEN_MIRROR_OF" "external:*")
to use _maven_find_prefixed_env:
local mirror_id=$(_maven_find_prefixed_env "${maven_mirror_prefix}" "MAVEN_MIRROR_ID" "mirror${counter}")
local mirror_url=$(_maven_find_prefixed_env "${maven_mirror_prefix}" "MAVEN_MIRROR_URL")
local mirror_of=$(_maven_find_prefixed_env "${maven_mirror_prefix}" "MAVEN_MIRROR_OF" "external:*")
because it can not read raw prefix here e.g. dev-one_MAVEN_MIRROR_URL even if this env exist. But it can read DEV_ONE_MAVEN_MIRROR_URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, but this was part of the original maven.sh code. It was not part of what I introduced. So whereas I think this could be an improvement, it shouldn't hold back this PR from getting processed.

Copy link
Contributor

Choose a reason for hiding this comment

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

If user tries to use MAVEN_REPOS "dev-one,qe-two" than it is not possible to load MAVEN_MIRROR_URL .. I just copied all "examples" and did not get too far :) .. I agree and will proceed without this, it is not show-stopper and I will create ticket for this.

@rcernich rcernich merged commit 2bdebac into jboss-openshift:master Dec 13, 2018
@errantepiphany errantepiphany deleted the CLOUD-2911 branch December 13, 2018 15:47
errantepiphany added a commit to jboss-container-images/jboss-kie-modules that referenced this pull request Dec 13, 2018
Merging this PR since jboss-openshift/cct_module#303 was merged, and this has to go along with it.
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

3 participants