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

fix(actions) print the deprecation action's kind warning only once #1037

Merged
merged 1 commit into from
Dec 12, 2022

Conversation

dduportal
Copy link
Contributor

Fix #1035

When writing #990 , I introduced a minor bug where the warning message telling the user about using a kind of action that is deprecated.

I forgot to update the internal config state, so the processing of template triggered the message at least a 2nd time (or more...).

This PR adds the required unit test that fails with the 0.40.0 behavior.

Test

To test this pull request, you can run the following commands:

# Build a local updatecli binary
go build -o ./dist/updatecli

# Remove the `version:` top level directive to avoid a failure, as local binary builds does not have a version
sed '/version:/d' ./updatecli/updatecli.d/golang.yaml | wc -l

# Execute the golang manifest, with only 1 warning this time
./dist/updatecli diff --config ./updatecli/updatecli.d/golang.yaml

result:

./dist/updatecli diff --config ./updatecli/updatecli.d/golang.yaml


+++++++++++
+ PREPARE +
+++++++++++

Loading Pipeline "./updatecli/updatecli.d/golang.yaml"
WARNING: The `pullrequests` keyword is deprecated in favor of `actions`, please update this manifest. Updatecli will continue the execution while trying to translate `pullrequests` to `actions`.
WARNING: The kind "github" for actions is deprecated in favor of 'github/pullrequest'

SCM repository retrieved: 1

# ...

Additional Information

  • Using the version top-level directive is blocking local builds and debug. We should set a default "dev" or "debug" version by default, that would be accepted. It can be cumbersome when debugging.

    • Should we remove it from the manifests? I can't remember the use case of specifying it?
  • The unit test does not cover the whole package code: room for improvement :)

    • There is a (minor) bug caught by unit tests (I did not added the failing unit tests here as it's another scope) that I'll try to fix tomorrow.

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@olblak
Copy link
Member

olblak commented Dec 12, 2022

I was just about to start debugging this issue and I saw your PR ❤️

I've run the case with a debugger: the double validation message appears because the manifest is templated.
Each time the template is rendered (e.g. twice), the validation happens on the newly update template.

Nice catch

Using the version top-level directive is blocking local builds and debug. We should set a default "dev" or "debug" version by default, that would be accepted. It can be cumbersome when debugging.

I agree, I also face the issue. The version is used by the updatecli manifest upgrade to check that we can upgrade a new version syntax and not downgrade a manifest. The problem here the variable "version.Version" is not defined at build time

@dduportal dduportal added bug Something isn't working actions labels Dec 12, 2022
@olblak
Copy link
Member

olblak commented Dec 12, 2022

I confirm that your PR also fix the updatecli manifest upgrade

⚠ Updatecli manifest change required
--- updatecli/updatecli.d/venom.yaml(old)
+++ updatecli/updatecli.d/venom.yaml(updated)
@@ -1,9 +1,9 @@
 name: Bump venom version
 pipelineid: 6a1c726e25648f93a45aa3b540ba8fd6307a62f92830404f55259bfbbc1b3c6f
-pullrequests:
+actions:
     default:
         title: '[updatecli] Bump Venom version to {{ source "latestVersion" }}'
-        kind: github
+        kind: github/pullrequest

@olblak olblak merged commit e103eda into updatecli:main Dec 12, 2022
@dduportal dduportal deleted the fix/gh-1035 branch June 18, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actions bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong warning message sent from Github resource of type source,condition,target
2 participants