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

Fixes the release 400 error on smoke tests #11

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

crdant
Copy link
Member

@crdant crdant commented Jun 30, 2023

TL;DR

Makes sure versions are aligned when running smoke tests

Details

Replaces hardcode major.minor.patch of 0.0.1 from the version used in the smoke action and replaces it with the version from the Helm chart's Chart.yaml. I thought that would be all that was required to fix the 400 on release, but it wasn't. So now the smoke test also updates Chart.yaml with the version that it creates to use for the test release. This required by the strict rules the vendor portal has for validating that the release version matches the chart version.

Ideally we could stash the build related tagging into the build metadata fragment of the semver instead of the prerelease version, and the vendor portal could ignore build metadata.

@crdant crdant force-pushed the crdant/fix/smoke-chart-version branch 2 times, most recently from 3f8cc28 to 700ac60 Compare July 2, 2023 13:59
Copy link
Member

@jdewinne jdewinne left a comment

Choose a reason for hiding this comment

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

I would do this different, and in the case of helm, automatically extract the app version from the Chart.yaml, vs having it dynamic.

smoke-test/action.yml Outdated Show resolved Hide resolved
if: inputs.version == ''
shell: bash
run: |
tarball=$(find ${YAML_DIR} -name "${CHART_NAME}*.tgz")
Copy link
Member

Choose a reason for hiding this comment

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

I guess this will break if the yaml-dir doesn't contain a helm chart?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, all the manipulation of the chart will break, I should make those conditional

Copy link
Member Author

Choose a reason for hiding this comment

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

made a few more things conditional

smoke-test/action.yml Outdated Show resolved Hide resolved
@crdant crdant force-pushed the crdant/fix/smoke-chart-version branch from 4937047 to 700ac60 Compare July 5, 2023 19:55
@xavpaice
Copy link
Member

@crdant this has been a while, is it still needing review or is it no longer required?

@crdant
Copy link
Member Author

crdant commented Sep 29, 2023

@crdant this has been a while, is it still needing review or is it no longer required?

I've generally moved away from our actions so I'm not sure. Happy to check it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants