Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

chore(argo-workflows): suggest non deprecated option #3113

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore(argo-workflows): suggest non deprecated option #3113

wants to merge 1 commit into from

Conversation

chansuke
Copy link

@chansuke chansuke commented Jan 9, 2025 •
edited
Loading

I was adding the SSO configuration and discovered that the depecated authMode was being suggested.

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

@chansuke chansuke changed the title chore: suggest non deprecated option chore(argo-workfkow): suggest non deprecated option Jan 9, 2025
@chansuke chansuke changed the title chore(argo-workfkow): suggest non deprecated option chore(argo-workfkows): suggest non deprecated option Jan 9, 2025
@chansuke chansuke changed the title chore(argo-workfkows): suggest non deprecated option chore(argo-workflows): suggest non deprecated option Jan 9, 2025
@chansuke chansuke marked this pull request as ready for review January 9, 2025 05:52
@yu-croco
Copy link
Collaborator

yu-croco commented Jan 9, 2025

Hi @chansuke , thank you for your PR.
Please handle items on your PR description.

I have bumped the chart version according to versioning
I have updated the documentation according to documentation
I have updated the chart changelog with all the changes that come with this pull request according to changelog.

@yu-croco yu-croco marked this pull request as draft January 9, 2025 05:57
@chansuke chansuke force-pushed the fix/suggest-authmodes branch from db516a9 to a0d5623 Compare January 9, 2025 08:00
@chansuke chansuke marked this pull request as ready for review January 9, 2025 08:00
@chansuke
Copy link
Author

chansuke commented Jan 9, 2025

@yu-croco I appreciate so much for your comment. I updated the description.

yu-croco
yu-croco reviewed Jan 9, 2025
View reviewed changes
charts/argo-workflows/Chart.yaml Outdated
Comment on lines 19 to 20
- kind: added
description: Support ephemeral credentials for s3 artifact repository
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the old one.

yu-croco
yu-croco reviewed Jan 9, 2025
View reviewed changes
charts/argo-workflows/values.yaml
@@ -711,7 +711,7 @@ server:

# SSO configuration when SSO is specified as a server auth mode.
sso:
# -- Create SSO configuration. If you set `true` , please also set `.Values.server.authMode` as `sso`.
# -- Create SSO configuration. If you set `true` , please also set `.Values.server.authModes` as `sso`.
Copy link
Collaborator

@yu-croco yu-croco Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think you need to handle below to update README, by running ./scripts/helm-docs.sh (instruction is in the link below).
Ref:

I have updated the documentation according to documentation

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I hit the command but the generation was falling because of this problem. Fixed and generated now.

@chansuke chansuke force-pushed the fix/suggest-authmodes branch from d83dec2 to daa88bd Compare January 9, 2025 09:37
@chansuke chansuke requested a review from yu-croco January 9, 2025 09:39
yu-croco
yu-croco previously approved these changes Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@yu-croco yu-croco left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. LGTM.

@yu-croco
Copy link
Collaborator

Hi @chansuke , can you please resolve conflict?

@chansuke
chore: suggest non deprecated option
a0421bf 
Signed-off-by: chansuke <chansuke0@gmail.com>
@chansuke chansuke force-pushed the fix/suggest-authmodes branch from 7312724 to a0421bf Compare January 14, 2025 22:02
@chansuke
Copy link
Author

@yu-croco
I appreciate your comment. I removed the old one and rebased the branch.

@yu-croco yu-croco enabled auto-merge (squash) January 15, 2025 00:31
yu-croco
yu-croco previously approved these changes Jan 15, 2025
View reviewed changes
@vladlosev
Copy link
Collaborator

The chart version needs a bump.

@yu-croco yu-croco dismissed their stale review January 15, 2025 14:27

need to fix for merging

@yu-croco
Copy link
Collaborator

yu-croco commented Jan 15, 2025 •
edited
Loading

ah resolving conflict was not correct, need to fix below 🙃

  • remove old changelog
  • bump version

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@yu-croco yu-croco yu-croco left review comments

@vladlosev vladlosev Awaiting requested review from vladlosev vladlosev is a code owner

@jmeridth jmeridth Awaiting requested review from jmeridth jmeridth is a code owner

@tico24 tico24 Awaiting requested review from tico24 tico24 is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
argo-workflows size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chansuke @yu-croco @vladlosev