feat: pre-pull Azure Stack's custom Hyperkube #1040
Conversation
💖 Thanks for opening your first pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix. Examples of commit messages with semantic prefixes: - |
I just blindly created this PR. Please let me know what validations do you usually run for this set of scripts. |
Codecov Report
@@ Coverage Diff @@
## master #1040 +/- ##
==========================================
+ Coverage 74.33% 74.36% +0.02%
==========================================
Files 131 131
Lines 18259 18277 +18
==========================================
+ Hits 13573 13591 +18
Misses 3905 3905
Partials 781 781 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is breaking for the Ubuntu distro scenario with not-Azure-Stack env
Before this change:
After this change:
|
Can't we just add |
+1 on @jackfrancis's suggestion. This should also work with the |
HYPERKUBE_URL="msazurestackdocker/hyperkube-amd64:v${KUBERNETES_VERSION}" | ||
else | ||
HYPERKUBE_URL="k8s.gcr.io/hyperkube-amd64:v${KUBERNETES_VERSION}" | ||
CONTAINER_IMAGE="k8s.gcr.io/cloud-controller-manager-amd64:v${KUBERNETES_VERSION}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all of this should be outside of the if block, right? The only conditional change is to HYPERKUBE_URL
, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the hyperkube image is different, cloud-controller-manager remains the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is, this implementation doesn't actually pre-pull the image in the Azure Stack case. The if should be:
if [[ $KUBERNETES_VERSION == *"azs"* ]]; then
HYPERKUBE_URL="msazurestackdocker/hyperkube-amd64:v${KUBERNETES_VERSION}"
else
HYPERKUBE_URL="k8s.gcr.io/hyperkube-amd64:v${KUBERNETES_VERSION}"
fi
Unless I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function extractHyperkube
calls pullContainerImage $CLI_TOOL ${HYPERKUBE_URL}
.
That clears your concern, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
/lgtm |
Congrats on merging your first pull request! 🎉🎉🎉 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, jadarsie The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* feat: pre-pull Azure Stack's custom Hyperkube * Changed approach from optional params to global variables * Update caller code instead of provisioning functions * Updated AZS hyperkube repository * Trust the pipeline * Updated AZURE_STACK_K8S_VERSIONS * Updated AZURE_STACK_K8S_VERSIONS * Add AzS prefix to KUBERNETES_VERSION's ARM param * cleanUpContainerImages * fixed armvariables_test * test * fixed build * Removed invalid chars, hopefully will get a Mac soon
Reason for Change:
Stores Azure Stack's custom Hyperkube build in the VHD image.
Issue Fixed:
Fixes #990
Requirements:
Notes: