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.

Sign up for GitHub

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

Jump to bottom

Make helm chart job only run if container is built ok #667

Merged
merged 8 commits into from
Nov 5, 2024
Merged

Make helm chart job only run if container is built ok #667

merged 8 commits into from
Nov 5, 2024

Conversation

stan-dot
Copy link
Contributor

Fixes #136

@stan-dot stan-dot added bug Something isn't working enhancement New feature or request github_actions Pull requests that update GitHub Actions code labels Oct 11, 2024
@stan-dot stan-dot self-assigned this Oct 11, 2024
@stan-dot stan-dot linked an issue Oct 11, 2024 that may be closed by this pull request
CI: Helm chart should not be created if the container creation fails #136
Closed
@codecov Codecov
Copy link

codecov bot commented Oct 11, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.35%. Comparing base (5abe34a) to head (05940c0).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #667   +/-   ##
=======================================
  Coverage   92.35%   92.35%           
=======================================
  Files          35       35           
  Lines        1793     1793           
=======================================
  Hits         1656     1656           
  Misses        137      137

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stan-dot
Copy link
Contributor Author

that is the culprit - might need to solve both issues in one go for this to pass
#411

@callumforrester
Copy link
Contributor

Please give this PR a sensible title

@stan-dot stan-dot changed the title try out by adding the needs field Make helm chart job only run if container is built ok Oct 14, 2024
@stan-dot stan-dot force-pushed the 136-ci-helm-chart-should-not-be-created-if-the-container-creation-fails branch from 62b1bb7 to dc0709f Compare October 16, 2024 13:48
@stan-dot stan-dot marked this pull request as ready for review October 16, 2024 13:49
DiamondJoseph
DiamondJoseph reviewed Oct 24, 2024
View reviewed changes
.github/workflows/_container.yml Outdated
Comment on lines 21 to 31
- name: Validate PEP 440 version compliance
run: |
ref="${{ github.ref_name }}"
my_regex=${{env.PEP_440_REGEX}}
if [[ "$ref" =~ $my_regex ]]; then
echo "PEP 440 compliant version: $ref"
else
echo "Invalid PEP 440 version: $ref"
exit 1
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes seem unrelated, why are they required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

otherwise it rans into #411 , so this PR fixes both,

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't fix #411 because if the user makes a release with a wrongly-formatted tag, the release will still appear in Github and PyPi(https://pypi.org/project/blueapi/#history). I don't think there's any harm in checking here, although it would be redundant if we could do something like this.

However, PEP440 actually allows for a wider range of error messages than helm (for instance, it allows 1.1a1 where helm would require 1.1-a1) so this check and error message aren't quite right. See https://helm.sh/docs/chart_best_practices/conventions/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well we convert to semver later ( - name: Convert PEP 440 version to SemVer) , is that ok?

Copy link
Contributor

@callumforrester callumforrester Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would be okay with removing the PEP440 check entirely and making a separate issue for adding SemVer check. I would not be okay with leaving the PEP440 check in since it won't catch any useful errors reported in #411.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not semver check in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would also be acceptable. It's only the PEP 440 check that I won't approve.

@stan-dot stan-dot force-pushed the 136-ci-helm-chart-should-not-be-created-if-the-container-creation-fails branch 3 times, most recently from 8cfdd2d to 036220a Compare November 1, 2024 13:56
callumforrester
callumforrester requested changes Nov 4, 2024
View reviewed changes
.github/workflows/_container.yml Outdated
token: ${{ secrets.GITHUB_TOKEN }}
id: install

- name: login to acr using helm
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: login to acr using helm
- name: login to gcr using helm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

.github/workflows/_container.yml Show resolved Hide resolved
@stan-dot stan-dot force-pushed the 136-ci-helm-chart-should-not-be-created-if-the-container-creation-fails branch from ef2ad15 to 58a32bd Compare November 4, 2024 16:31
callumforrester
callumforrester approved these changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

LGTM!

@stan-dot stan-dot merged commit 1c50a4b into main Nov 5, 2024
28 checks passed
@stan-dot stan-dot deleted the 136-ci-helm-chart-should-not-be-created-if-the-container-creation-fails branch November 5, 2024 09:51
@stan-dot stan-dot mentioned this pull request Nov 15, 2024
Merged
DiamondJoseph added a commit that referenced this pull request Nov 15, 2024
@stan-dot @DiamondJoseph
713 quickfix too restrictive semver check - fixing the ref/version pa…
86b5698 
…rsing (#715)

Fixes #667

---------

Co-authored-by: Joseph Ware <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@callumforrester callumforrester callumforrester approved these changes

@abbiemery abbiemery Awaiting requested review from abbiemery

@dan-fernandes dan-fernandes Awaiting requested review from dan-fernandes

@DiamondJoseph DiamondJoseph Awaiting requested review from DiamondJoseph

Assignees

@stan-dot stan-dot

Labels
bug Something isn't working enhancement New feature or request github_actions Pull requests that update GitHub Actions code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI: Helm chart should not be created if the container creation fails
3 participants
@stan-dot @callumforrester @DiamondJoseph