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.

Sign up for GitHub

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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