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

docs: add available configuration options for rollouts controller. Fixes #1914 #1921

Conversation

hcelaloner
Copy link

It seems that docs are not listing available configuration options for the controller, this is a just simple attempt to list options as in ArgoCD Server Configuration Parameters docs.

Refs: #1914
Signed-off-by: Celal Öner [email protected]

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Base: 81.55% // Head: 82.42% // Increases project coverage by +0.86% 🎉

Coverage data is based on head (c26e78d) compared to base (93c2520).
Patch has no changes to coverable lines.

❗ Current head c26e78d differs from pull request most recent head 9e44a75. Consider uploading reports for the commit 9e44a75 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1921      +/-   ##
==========================================
+ Coverage   81.55%   82.42%   +0.86%     
==========================================
  Files         124      119       -5     
  Lines       18931    16913    -2018     
==========================================
- Hits        15439    13940    -1499     
+ Misses       2702     2282     -420     
+ Partials      790      691      -99     
Impacted Files Coverage Δ
utils/unstructured/unstructured.go 55.93% <0.00%> (-18.40%) ⬇️
pkg/kubectl-argo-rollouts/cmd/lint/lint.go 67.90% <0.00%> (-17.95%) ⬇️
ingress/ingress.go 60.60% <0.00%> (-13.40%) ⬇️
service/service.go 68.37% <0.00%> (-10.14%) ⬇️
analysis/controller.go 52.17% <0.00%> (-9.37%) ⬇️
pkg/kubectl-argo-rollouts/info/analysisrun_info.go 82.60% <0.00%> (-7.40%) ⬇️
experiments/controller.go 67.28% <0.00%> (-4.14%) ⬇️
utils/replicaset/canary.go 89.26% <0.00%> (-3.46%) ⬇️
utils/ingress/wrapper.go 89.25% <0.00%> (-3.08%) ⬇️
rollout/controller.go 77.75% <0.00%> (-3.02%) ⬇️
... and 51 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rollouts-controller [flags]
```

```
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it is a good idea to put the following command options in the doc as the content could be staled over time. I think the example in the PR is good enough.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you are right. On the negative side, when a new option is added/removed, this section could contain stale information in the future. However, on the other hand, someone can see all available configuration options from docs. If this seems unnecessary I can completely remove this part and only gave an example of changing log format.

By the way, how about writing something like as follows after the example as an alternative

You can list all available cli options for rollouts controller using the following command:

docker run --rm -it quay.io/argoproj/argo-rollouts:v1.2.0 -h

Copy link
Member

Choose a reason for hiding this comment

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

The alternative looks much better. Please go ahead with the proposed change. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

First, thanks for the comments and review 🙂. I changed the docs as we talked but some checks are failing. I guess it is not related to the changes proposed in this pr, or is there actually something I should fix?

@hcelaloner hcelaloner force-pushed the docs/add-controller-configuration-options branch from 67d3900 to 6d7f3ea Compare March 24, 2022 03:45
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@@ -26,6 +26,23 @@ This will create a new namespace, `argo-rollouts`, where Argo Rollouts controlle
You can find released container images of the controller at [Quay.io](https://quay.io/repository/argoproj/argo-rollouts?tab=tags). There are also old releases
at Dockerhub, but since the introduction of rate limiting, the Argo project has moved to Quay.

### Configuration Options

For instance, logging format can be easily set as `json` via passing the following additional command-line argument in Argo Rollouts Controller Deployment:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For instance, logging format can be easily set as `json` via passing the following additional command-line argument in Argo Rollouts Controller Deployment:
For instance, logging format can be easily set as `json` via passing the following additional command-line argument in ArgoRollouts controller deployment:

- --logformat=json
````

You can list all available CLI options for rollouts controller using the following command:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can list all available CLI options for rollouts controller using the following command:
You can list all the available CLI options for the rollouts controller using the following command:

@github-actions
Copy link
Contributor

This PR is stale because it has been open 90 days with no activity.

@hcelaloner hcelaloner force-pushed the docs/add-controller-configuration-options branch from c26e78d to d49e172 Compare November 9, 2022 07:20
@hcelaloner hcelaloner force-pushed the docs/add-controller-configuration-options branch from d49e172 to 9e44a75 Compare November 9, 2022 07:22
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@hcelaloner
Copy link
Author

Hi everyone, I tried to update the branch and apply all the pr suggestions. Could you review it again when you have time and merge it if it is okay for the project?

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Go Published Test Results

1 798 tests   1 798 ✔️  2m 32s ⏱️
   102 suites         0 💤
       1 files           0

Results for commit 9e44a75.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

E2E Tests Published Test Results

    2 files      2 suites   1h 31m 25s ⏱️
  89 tests   84 ✔️ 3 💤 2
180 runs  172 ✔️ 6 💤 2

For more details on these failures, see this check.

Results for commit 9e44a75.

@hcelaloner
Copy link
Author

Thanks to everyone who helped in the past. I'm closing this pr, it seems that there will be no activity for this pr.

@hcelaloner hcelaloner closed this Feb 3, 2023
@reilly3000
Copy link

I'm not clear on why this was closed. I definitely could have used this documentation today!

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.

4 participants