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

Remove hardcoded version of chaoscenter in pre-hook job in litmus-agent chart #329 #330

Merged
merged 7 commits into from
Jan 11, 2024

Conversation

Calvinaud
Copy link
Contributor

What this PR does / why we need it:

The version of the chaoscenter was hardcoded in the template of the litmus-agent charts that was posing problem when deploying with different version of chaoscenter. The version is now a variable.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

Calvin Audier added 2 commits August 25, 2023 19:07
- Use a variable for the version value in the hook-pre-install-job templates
- Add variable to the values.yaml with a default value of 3.0.0-beta8
- Bump chart to version 0.2.1

Signed-off-by: Calvin Audier <[email protected]>
Copy link
Member

@gdsoumya gdsoumya left a comment

Choose a reason for hiding this comment

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

The main reason to probably hardcode the version was to keep it compatible with the version of the chaos center. There might be other changes needed to the chart/code to work with a different version of the chaos center as things are changing quite rapidly. This modification gives users the impression that this particular chart can work with a different version too which might not be the case.

Tagging @Jonsy13 to confirm if this change is safe to make

@Calvinaud
Copy link
Contributor Author

Not sure either, but the hardcoded version mean we cannot deploy in 2.14.0 which is still at the moment the no beta version.

In our deployment, we are working with all images in 2.14.0 and we updated the version manually in the agent configmap, will keep updated if we found any issues.

@Jasstkn
Copy link
Member

Jasstkn commented Aug 31, 2023

The main reason to probably hardcode the version was to keep it compatible with the version of the chaos center. There might be other changes needed to the chart/code to work with a different version of the chaos center as things are changing quite rapidly. This modification gives users the impression that this particular chart can work with a different version too which might not be the case.

Tagging @Jonsy13 to confirm if this change is safe to make

Hey. Any update here? For me this change makes sense as long as default is stays the same.

@gdsoumya
Copy link
Member

gdsoumya commented Aug 31, 2023

Not sure either, but the hardcoded version mean we cannot deploy in 2.14.0 which is still at the moment the no beta version.

In our deployment, we are working with all images in 2.14.0 and we updated the version manually in the agent configmap, will keep updated if we found any issues.

The agent helm chart is only supported for 3.x litmus version not 2.x, litmus doesn't provide any support for 2.x in this helm chart. Ref: https://github.com/litmuschaos/litmus-helm#install-external-agent

@Calvinaud
Copy link
Contributor Author

Calvinaud commented Aug 31, 2023

Even if the version 2.x is not supported shouldn't the version still not be hardcoded to have the possible to overwrite it to have the risk of hardcoded incompatibility between chaoscenter and litmus agent (even between two 3.x version) ?

This modification gives users the impression that this particular chart can work with a different version too which might not be the case.

Should I add an extra comment for this variable to say "Change at your own risk" (and add it in the description of the variable) ?

@gdsoumya
Copy link
Member

@Calvinaud I think the current approach is to have a separate version of the litmus agent that works for a specific 3.x version of chaos center so if you are using a specific version you need to use the specific chart version corresponding to that.

@Jonsy13
Copy link
Contributor

Jonsy13 commented Aug 31, 2023

I think it should be fine to have version to be provided via values.yml as long as we are able to make sure that users know that this agent is not compatible with any other version except 3.x at the moment. @gdsoumya is correct, this was done intentionally. Tagging @ispeakc0de as well to have a look once.

@Calvinaud
Copy link
Contributor Author

think the current approach is to have a separate version of the litmus agent that works for a specific 3.x version of chaos center so if you are using a specific version you need to use the specific chart version corresponding to that.

Just a random idea:
If the goal is to bind a version of chaoscenter to the litmus agent chart. Is using the appVersion of the Charts.yaml as the default value for the VERSION (with the possibility to overwrite in values.yaml) just like the image tag a good idea ?

@Calvinaud
Copy link
Contributor Author

Calvinaud commented Sep 12, 2023

Any opinion on my last comment ?

@Calvinaud
Copy link
Contributor Author

Hello,
@Jasstkn @gdsoumya @Jonsy13 @ispeakc0de
Retagging you in this PR to have the review/approval.

@Jasstkn
Copy link
Member

Jasstkn commented Jan 1, 2024

think the current approach is to have a separate version of the litmus agent that works for a specific 3.x version of chaos center so if you are using a specific version you need to use the specific chart version corresponding to that.

Just a random idea: If the goal is to bind a version of chaoscenter to the litmus agent chart. Is using the appVersion of the Charts.yaml as the default value for the VERSION (with the possibility to overwrite in values.yaml) just like the image tag a good idea ?

I don't have a strong opinion about that.

@nwinstel-insight
Copy link

The main reason to probably hardcode the version was to keep it compatible with the version of the chaos center. There might be other changes needed to the chart/code to work with a different version of the chaos center as things are changing quite rapidly. This modification gives users the impression that this particular chart can work with a different version too which might not be the case.

Tagging @Jonsy13 to confirm if this change is safe to make

Does this mean 3.0.0-beta8 is the appropriate version for 3.2.0?

@Calvinaud
Copy link
Contributor Author

Hello, @gdsoumya are you still requesting changes in this PR ?

@gdsoumya gdsoumya merged commit 31a0a30 into litmuschaos:master Jan 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Cannot deploy litmus-agent with older version of chaos center
6 participants