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

SONAR-18974 fix ApplicationNodes capital A typo #586

Merged

Conversation

jCOTINEAU
Copy link
Collaborator

No description provided.

@jCOTINEAU jCOTINEAU force-pushed the feature/jc/SONAR-18974-fix-application-nodes-capital-a-typo branch from 88311ce to 44bab2c Compare November 15, 2024 10:36
@jCOTINEAU jCOTINEAU changed the title DAFT SONAR-18974 fix ApplicationNodes capital A typo SONAR-18974 fix ApplicationNodes capital A typo Nov 18, 2024
Copy link
Collaborator

@carminevassallo carminevassallo left a comment

Choose a reason for hiding this comment

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

This is such a great PR! One of the best on the helm chart so far!

The local functional validation does not show any critical issue, plus the fixtures are already confirming the absence of regression 👏

I have a small comment on using the deepMerge function when it's not needed. Apart from that, how do we communicate the deprecation to users? Setting (DEPRECATED) to all ApplicationNodes values and then replicate all the parameters is not option, but shall we spend some words on the README about it? Plus, shall we leverage Warnings?

@jCOTINEAU
Copy link
Collaborator Author

I have a small comment on using the deepMerge function when it's not needed. Apart from that, how do we communicate the deprecation to users? Setting (DEPRECATED) to all ApplicationNodes values and then replicate all the parameters is not option, but shall we spend some words on the README about it? Plus, shall we leverage Warnings?

I agree, warning is a good option, and I'll add a quick section in the readme.

@jCOTINEAU jCOTINEAU force-pushed the feature/jc/SONAR-18974-fix-application-nodes-capital-a-typo branch from 1cfd0f9 to 989f04a Compare November 21, 2024 09:35
Copy link
Collaborator

@carminevassallo carminevassallo left a comment

Choose a reason for hiding this comment

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

I think we can merge now! 🚀

I edited slightly the readme and warning message. Feel free to revise them if needed.

Please do not forget to acknowledge the issues on SQ!

@jCOTINEAU jCOTINEAU force-pushed the feature/jc/SONAR-18974-fix-application-nodes-capital-a-typo branch 3 times, most recently from 5d260aa to 777c76a Compare November 21, 2024 16:09
@jCOTINEAU jCOTINEAU force-pushed the feature/jc/SONAR-18974-fix-application-nodes-capital-a-typo branch from 777c76a to c53f7a9 Compare November 22, 2024 08:13
Copy link

@jCOTINEAU jCOTINEAU merged commit 46fcce0 into master Nov 22, 2024
9 checks passed
@jCOTINEAU jCOTINEAU deleted the feature/jc/SONAR-18974-fix-application-nodes-capital-a-typo branch November 22, 2024 08:51
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.

2 participants