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

ansible/helm: Rename max-workers to max-concurrent-reconciles #3452

Merged

Conversation

varshaprasad96
Copy link
Member

@varshaprasad96 varshaprasad96 commented Jul 17, 2020

Description of the change:
This commit:

  • Adds --max-concurrent-reconciles flag to helm binary
  • Rename MaxWorkers to MaxConcurrentReconciles in ansible/helm
    implementations.

Fixes: #3360

Motivation for the change:
Modify operator flags in ansible/helm operators to match controller-runtime constructs.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Comment on lines 3 to 4
Add "--max-concurrent-reconciles" flag to helm binary. Rename "max-workers" to "max-concurrent-reconciles"
in ansible/helm implementations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename "max-workers" to "max-concurrent-reconciles" in ansible/helm implementations.

Is this referring to renaming/replacing the flag? In which case this is not an addition.
The verb "Rename" would be better suited to the Changed section.

Or is this referring to renaming the ansible controller Options field from MaxWorkers to MaxConcurrentReconcilers? I don't think that's a user facing change, and so we don't need to mention that in the CHANGELOG.

Also can you add a one line explanation for what --max-concurrent-reconciles does, or link to existing docs if we have those.

Copy link
Member Author

@varshaprasad96 varshaprasad96 Jul 17, 2020

Choose a reason for hiding this comment

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

Add "--max-concurrent-reconciles" flag to helm binary - meant the adding of a new flag, so that later we could deprecate --max-workers.
For Rename "max-workers" to "max-concurrent-reconciles" in ansible/helm implementations., I meant renaming the ansible controller Options field from MaxWorkers to MaxConcurrentReconcilers. Ill remove the second part from changelog.

pkg/ansible/watches/watches.go Outdated Show resolved Hide resolved
cmd/helm-operator/main.go Outdated Show resolved Hide resolved
cmd/helm-operator/main.go Outdated Show resolved Hide resolved
pkg/ansible/watches/watches_test.go Show resolved Hide resolved
@@ -144,13 +144,13 @@ func main() {
}
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jul 17, 2020

Choose a reason for hiding this comment

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

Missing update the helm doc: https://github.com/operator-framework/operator-sdk/blob/master/website/content/en/docs/building-operators/helm/reference/advanced_features.md#changing-the-concurrent-worker-count

Also, could you please replace its example for to use the new layout? We have no longer the exec-entrypoint.

to use it we need to update the config/manager/manager.yaml with or run $ helm-operator --max-concurrent-reconciles=3

 spec:
      containers:
      - image: {{ .Image }}
        args:
        - "--enable-leader-election"
        - "--max-concurrent-reconciles=3"
        name: manager

Copy link
Member

Choose a reason for hiding this comment

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

@varshaprasad96 @camilamacedo86

This section was pre-emptively updated and wordsmith-ed here:

https://github.com/operator-framework/operator-sdk/pull/3326/files#diff-47162e7ecac88f3c6f2c91e49dcd7757R91-R105

It would be better to make that update in this PR, so feel free to steal that section.

Copy link
Contributor

Choose a reason for hiding this comment

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

merged the helm doc to help its updated.
PS.: I update the doc and forgot :-) tks

Copy link
Member Author

Choose a reason for hiding this comment

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

Made a small addition to the existing doc.

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

just a few nits;

  • rebase with the master (get docs)
  • update the helm doc with the change
  • update the fragment

Otherwise, it shows great 👍

This commit:
- Adds --max-concurrent-reconciles flag to helm binary
- Rename MaxWorkers to MaxConcurrentReconciles in ansible/helm
  implementations.
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2020
@joelanford joelanford mentioned this pull request Jul 20, 2020
92 tasks
@varshaprasad96 varshaprasad96 merged commit 236aa84 into operator-framework:master Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
6 participants