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

doc: add extra info over ansible watches and reconcile period #3384

Merged
merged 3 commits into from
Jul 15, 2020
Merged

doc: add extra info over ansible watches and reconcile period #3384

merged 3 commits into from
Jul 15, 2020

Conversation

camilamacedo86
Copy link
Contributor

Description of the change:

  • add info over reconcilePeriod similar to the sync period attribute of manager.Options to make clear that users should avoid small periods
  • clarifies that users are able to create many CRD's and use the APIs to persuade the concepts such as single responsibility, cohesion and encapsulation which would bring more flexibility and optimization since they are able to do configurations per GKV via the watches file.

Motivation for the change:

#3364

@camilamacedo86 camilamacedo86 mentioned this pull request Jul 10, 2020
Closed
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 11, 2020
asmacdo
asmacdo reviewed Jul 13, 2020
View reviewed changes
Copy link
Member

@asmacdo asmacdo left a comment

Choose a reason for hiding this comment

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

This value can be counter-intuitive, so I tried explaining it in a different way. WDYT?

website/content/en/docs/ansible/development-tips.md Outdated Show resolved Hide resolved
website/content/en/docs/ansible/development-tips.md Show resolved Hide resolved
website/content/en/docs/ansible/reference/watches.md Outdated Show resolved Hide resolved
fabianvf
fabianvf reviewed Jul 14, 2020
View reviewed changes
Copy link
Member

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

Looks good to me once Austin's comments are addressed

website/content/en/docs/ansible/development-tips.md Show resolved Hide resolved
@camilamacedo86
Copy link
Contributor Author

HI @fabianvf and @asmacdo,

Tks for the review and help with. See wdyt know? Is it ok? Could we move forward here?

asmacdo
asmacdo reviewed Jul 15, 2020
View reviewed changes
website/content/en/docs/ansible/development-tips.md Outdated
@@ -498,7 +500,7 @@ operator. The `meta` fields can be accesses via dot notation in Ansible as so:
```yaml
---
- debug:
msg: "name: {{ meta.name }}, {{ meta.namespace }}"
msg: "name: {{ meta.name }}, {{ meta.namespace }}"f
Copy link
Member

Choose a reason for hiding this comment

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

typo?

jmrodri
jmrodri approved these changes Jul 15, 2020
View reviewed changes
Copy link
Member

@jmrodri jmrodri 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 15, 2020
@camilamacedo86 camilamacedo86 merged commit 27398bb into operator-framework:master Jul 15, 2020
@camilamacedo86 camilamacedo86 deleted the doc-reconcile branch July 15, 2020 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri approved these changes

@asmacdo asmacdo asmacdo approved these changes

@jmccormick2001 jmccormick2001 Awaiting requested review from jmccormick2001

@fabianvf fabianvf Awaiting requested review from fabianvf

Assignees

@jmrodri jmrodri

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@camilamacedo86 @jmrodri @asmacdo @fabianvf @openshift-ci-robot