refactors helm targets to avoid duplication and improve logging #2608
Conversation
eks-distro-bot
added
do-not-merge/work-in-progress
size/M
jaxesn
force-pushed
the
jgw/packages-improv
branch
from
e54ea82 to
1a41729
Compare
eks-distro-bot
added
size/L
jaxesn
force-pushed
the
jgw/packages-improv
branch
4 times, most recently
from
5707dad to
998cea3
Compare
jaxesn
changed the title
jaxesn
force-pushed
the
jgw/packages-improv
branch
from
998cea3 to
839815a
Compare
There was a problem hiding this comment.
I validated a few Prow results, some include multiple helm charts (eks-anywhere-packages, and metallb), and some include single helm chart (prometheus, hello-eks-anywhere). It appeared that helm charts are getting built successfully using names consistent as before.
| @@ -12,8 +12,6 @@ HAS_LICENSES=false | |||
|
|
|||
| HAS_HELM_CHART=true | |||
| HELM_DIRECTORY=charts/hello-eks-anywhere | |||
| HELM_DESTINATION_REPOSITORY=$(REPO) | |||
There was a problem hiding this comment.
Nil: I know this repo doesn't need to specify HELM_CHART_NAMES due to how the repo was set up. I just find it easier to follow through if it's consistent with all other projects.
|
|
||
| # Adds a 2nd tag to the helm chart for the bundle-release jobs. | ||
| if [[ "${IMAGE_REGISTRY}" != *"public.ecr.aws"* ]]; then | ||
| MANIFEST=$(aws ecr batch-get-image --repository-name "$HELM_DESTINATION_REPOSITORY" --image-ids imageTag=${SEMVER} --query "images[].imageManifest" --output text) | ||
| MANIFEST=$(aws ecr batch-get-image --repository-name "$HELM_DESTINATION_REPOSITORY" --image-ids imageDigest=${DIGEST} --query "images[].imageManifest" --output text) |
There was a problem hiding this comment.
I don't see an issue with this update, though didn't find an example to validate the output of it. So just double check to be safe.
There was a problem hiding this comment.
good call out, i tried this one manually locally since presubmits do not actually push.
| @@ -799,22 +802,33 @@ binary-builder/cgo/%: USE_DOCKER_FOR_CGO_BUILD=$(shell command -v docker &> /dev | |||
| helm/pull: | $$(ENABLE_LOGGING) | |||
| @$(BUILD_LIB)/helm_pull.sh $(HELM_PULL_LOCATION) $(HELM_REPO_URL) $(HELM_PULL_NAME) $(REPO) $(HELM_DIRECTORY) $(CHART_VERSION) $(COPY_CRDS) | |||
|
|
|||
| .PHONY: %/helm/copy %/helm/require %/helm/replace %/helm/build %/helm/push | |||
| skopeo manifest-digest ${TMPFILE} | ||
| else | ||
| >&2 echo "Not Found!" | ||
| fi |
There was a problem hiding this comment.
it might be nice to someday move away from skopeo dependency to oras, but that is just me.
jaxesn
force-pushed
the
jgw/packages-improv
branch
from
839815a to
397afcf
Compare
jaxesn
force-pushed
the
jgw/packages-improv
branch
4 times, most recently
from
27fdcbb to
db7d5f7
Compare
jaxesn
force-pushed
the
jgw/packages-improv
branch
from
db7d5f7 to
faa44ed
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhay-krishna 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 |
Issue #, if available:
Description of changes:
When helm chart building was originally added it only supported 1 chart per project. When we started adding projects that had multiple charts, we started a pattern of duplicating the helm targets in the individual project Makefiles. This PR aims to remove all that as well as clean up the output from the build scripts like we did for most of our others, aka removing
set -x.I split this into two commits to make it easier to compare the results from the "existing" code vs my changes. I tried to keep my changes in the first commit to a minimum, I couldnt help myself a bit in the helm_require script so please go thru it pretty good. I then ran my second commit changes locally for all projects with helm charts to compare the new output with the old.
To implement support for multiple charts I took a similar approach to how we do image builds by setting CHART_NAMES in the project make file and turning those targets into wildcard targets.
The key things to check for are the PACKAGE_DEPENENDICES which used to be env vars set on the script, now are proper makefiles and occasionally overridden. FORCE_SCHEMA_JSON_FILE is another that is handle similarly. Most of the project changes are pretty minimal, there are just a few that actual build multiple charts.
Please also review some of the prow output for some of the key projects.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.