Skip to content

Commit

Permalink
fix(eks): reuse chart name as chart dir for helmchart deployment from…
Browse files Browse the repository at this point in the history
… OCI repository (#23392)

Fixes #22341

It migrates assigning chart_dir from release name to chart name, to be consistent with helm-pull's target directory. 

I also added to integration test yet another chart deployment of a different chart. Why it's separate? Because of the way of how testing flow is working - if i understand it correctly it provisions previous snapshot, and finally after that provisions a new version of assets and test code **on top of existing**. Thanks to that i'm unable to cover my case in existing chart deployment, because when i change release name it conflicts with previously deployed (during old snapshot provisioning) chart (`UPDATE_FAILED (The following resource(s) failed to update: [Clustercharttestocichart9C188967]. ): Received response status [FAILED] from custom resource. Message returned: Error: b'Release "s3-chart-release" does not exist. Installing it now.\nError: rendered manifests contain a resource that already exists. Unable to continue with install: ServiceAccount "ack-s3-controller" in namespace "ack-system" exists and cannot be imported into the current release: invalid ownership metadata; annotation validation error: key "meta.helm.sh/release-name" must equal "s3-chart-release": current value is "s3-chart"\n'`).

Actually i successfully ran the modified integration test, but i didn't commit changes caused by yarn integ command, because i don't know which i have to commit.

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
psemeniuk committed Jan 26, 2023
1 parent 660198b commit 070f5ec
Show file tree
Hide file tree
Showing 41 changed files with 479 additions and 370 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def helm_handler(event, context):

if repository is not None and repository.startswith('oci://'):
tmpdir = tempfile.TemporaryDirectory()
chart_dir = get_chart_from_oci(tmpdir.name, release, repository, version)
chart_dir = get_chart_from_oci(tmpdir.name, repository, version)
chart = chart_dir

helm('upgrade', release, chart, repository, values_file, namespace, version, wait, timeout, create_namespace)
Expand Down Expand Up @@ -123,7 +123,7 @@ def get_oci_cmd(repository, version):
return cmnd


def get_chart_from_oci(tmpdir, release, repository = None, version = None):
def get_chart_from_oci(tmpdir, repository = None, version = None):

cmnd = get_oci_cmd(repository, version)

Expand All @@ -135,7 +135,9 @@ def get_chart_from_oci(tmpdir, release, repository = None, version = None):
output = subprocess.check_output(cmnd, stderr=subprocess.STDOUT, cwd=tmpdir, shell=True)
logger.info(output)

return os.path.join(tmpdir, release)
# effectively returns "$tmpDir/$lastPartOfOCIUrl", because this is how helm pull saves OCI artifact.
# Eg. if we have oci://9999999999.dkr.ecr.us-east-1.amazonaws.com/foo/bar/pet-service repository, helm saves artifact under $tmpDir/pet-service
return os.path.join(tmpdir, repository.rpartition('/')[-1])
except subprocess.CalledProcessError as exc:
output = exc.output
if b'Broken pipe' in output:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def helm_handler(event, context):

if repository is not None and repository.startswith('oci://'):
tmpdir = tempfile.TemporaryDirectory()
chart_dir = get_chart_from_oci(tmpdir.name, release, repository, version)
chart_dir = get_chart_from_oci(tmpdir.name, repository, version)
chart = chart_dir

helm('upgrade', release, chart, repository, values_file, namespace, version, wait, timeout, create_namespace)
Expand Down Expand Up @@ -123,7 +123,7 @@ def get_oci_cmd(repository, version):
return cmnd


def get_chart_from_oci(tmpdir, release, repository = None, version = None):
def get_chart_from_oci(tmpdir, repository = None, version = None):

cmnd = get_oci_cmd(repository, version)

Expand All @@ -135,7 +135,7 @@ def get_chart_from_oci(tmpdir, release, repository = None, version = None):
output = subprocess.check_output(cmnd, stderr=subprocess.STDOUT, cwd=tmpdir, shell=True)
logger.info(output)

return os.path.join(tmpdir, release)
return os.path.join(tmpdir, repository.rpartition('/')[-1])
except subprocess.CalledProcessError as exc:
output = exc.output
if b'Broken pipe' in output:
Expand Down
Binary file not shown.

This file was deleted.

Loading

0 comments on commit 070f5ec

Please sign in to comment.