Modify sed command matching rule while updating version in Chart.yaml #194
Conversation
Codecov Report
@@ Coverage Diff @@
## main #194 +/- ##
=======================================
Coverage 78.66% 78.66%
=======================================
Files 17 17
Lines 2133 2133
=======================================
Hits 1678 1678
Misses 304 304
Partials 151 151
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
LGTM.
This PR is to fix failure for uploading assets when we release v0.5.1. @srikartati I think we need to release v0.5.2 for this fix, right?
|
I also think a new release will be helpful because there is a PR on Antrea with changes in flow-visibility documentation needs ipfix-collector.yaml showing in released assets. |
heanlan
changed the title
Sure, we can create a new tag for this. |
hack/generate-manifest-collector.sh
Outdated
| @@ -116,7 +116,7 @@ fi | |||
|
|
|||
| if [ "$MODE" == "release" ]; then | |||
| # update the Chart version | |||
| version=$(sed -n 's/v\(.*\)-dev/\1/p' $THIS_DIR/../VERSION) | |||
| version=$(sed -n 's/v\(.*\)/\1/p' $THIS_DIR/../VERSION) | |||
There was a problem hiding this comment.
In the case of "dev" mode, what version would you use?
There was a problem hiding this comment.
-
release mode
I realized line 119-121 actually make no effect. They are triggered by the release action. The changes to Chart.yaml file will neither go into the "Source Code zip" under "assets" nor the releasing branch.
If we want to make sure in the Source Code zip, the version is the same with the release tag, I think manually change the file would work, like how we change VERSION. I can't think of a good way to automatically make this change into the source code zip. -
dev mode
As for the dev mode, I can let the script to update it according to VERSION every time we generate manifest, (but if we change VERSION and do not generate manifest right away, the two "version" will not be the same during this time). Or we can also manually change it when we change dev VERSION.
That's to say, in the ideal case, the version in Chart.yaml will always be the same with VERSION in the same branch. Do you think manually change the version is acceptable?
There was a problem hiding this comment.
I just meant you could have the following line for the dev mode. Just to give an indication for the user if they are using the collector from main branch ToT. Do you see anything wrong?
version=$(sed -n 's/v\(.*\)-dev/\1/p' $THIS_DIR/../VERSION)
There was a problem hiding this comment.
Yes, it will work. What about release? Do we want the version to be the release tag in the source code zip? If so, I think we need to manually change it before release.
There was a problem hiding this comment.
I am confused about the manual change. Releases are done on release branch, where the VERSION file is changed before to reflect the tag, so the script should pick the correct tag. Not sure why will the VERSION file be different in the zipped source code.
There was a problem hiding this comment.
My bad. I didn't make it clear. The VERSION file is the same in the branch and the zipped source code. But the modification to Chart.yaml is triggered in this order:
manually create a new release -> upload_release_assets.yml -> generate-manifest-collector.sh -> if [ "$MODE" == "release" ]; then:
so the version in Chart.yaml hasn't been changed before we manually create the new release, before the source code been zipped. I have tested on my branch, in the zipped source code, the version in Chart.yaml is different from the VERSION.
There was a problem hiding this comment.
We are changing the chart version in the checked-out code in github CI and it's not checked into the branch. Is this the issue?
This version has no significance right? Does it have to be in the format x.x.x? I see that ipfix-collector version in the ToT code is latest.. we only have it's version in release assets. We can just stick with the same if possible.
There was a problem hiding this comment.
I see that it is a must: https://helm.sh/docs/topics/charts/#the-chartyaml-file
Let's just use 0.0.0 and do not change it. I feel changing this manually for every release is cumbersome.
(Edited) Or make a check for Chart.yaml in make manifest to make sure that it coincides with the version in VERSION. Person who is changing the version for release branch will take care of it.
There was a problem hiding this comment.
Sounds good. Let's make it 0.0.0 for now.
This PR is to fix the error in upload-release-assets job.
The change is made in the section where we update the version value in Chart.yaml by the value in "go-ipfix/VERSION".
In the original script, the sed command will match string in the form:
vx.x.x-dev. Now changed to match withvx.x.x.When I wrote the script, the version in "go-ipfix/VERSION" is
v0.6.0-dev, so I thought that is the form I need to match. I didn't realize the version will be changed to the form likev0.5.1before release. Sorry about creating the issue.