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

same standard for all proposals #1929

Merged
merged 11 commits into from
Oct 9, 2019

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Sep 17, 2019

Description of the change:

  • Update the template in order to use the OCP one.
  • Update the status for the current one and add the ref links

Motivation for the change:
Closes: #1910

@camilamacedo86 camilamacedo86 removed the request for review from joelanford September 17, 2019 14:01
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 17, 2019
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.

Thanks @camilamacedo86!

I think we should:

  1. Agree what the valid status states are and use those consistently (might need to capitalize or lowercase everything)
  2. Document these states in the README with a short description of each.
  3. Document expectations in the README about keeping proposals up-to-date and adding links when appropriate (doc links when complete, issues/PRs when in progress, deprecated, or removed, etc).

doc/proposals/TEMPLATE.md Outdated Show resolved Hide resolved
doc/proposals/cli-ux-phase1.md Outdated Show resolved Hide resolved
doc/proposals/helm-operator.md Outdated Show resolved Hide resolved
doc/proposals/scorecard-plugin-system.md Outdated Show resolved Hide resolved
doc/proposals/sdk-code-annotations.md Outdated Show resolved Hide resolved
doc/proposals/tls-utilities.md Outdated Show resolved Hide resolved
doc/proposals/improved-scorecard-config.md Outdated Show resolved Hide resolved
doc/proposals/leader-for-life.md Outdated Show resolved Hide resolved
doc/proposals/metering-operator-metrics.md Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 17, 2019
@camilamacedo86
Copy link
Contributor Author

Hi @joelanford,

all changes requested are done... could we move forward with as it is and close the Issue?
PS.: we can add any additional info or change in a second PR. WDYT?

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 19, 2019
@camilamacedo86
Copy link
Contributor Author

/test e2e-aws-ansible

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.

If we're going to add doctoc and suggest it in the template, I think we also need:

  1. A make target to make it easy to regenerate
  2. To ensure that the sanity test fails if doctoc is not properly regenerated.

doc/proposals/tls-utilities.md Outdated Show resolved Hide resolved
doc/proposals/ansible-operator-status.md Outdated Show resolved Hide resolved
@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Sep 19, 2019

Hi @joelanford,

Regards:

If we're going to add doctoc and suggest it in the template, I think we also need:

A make target to make it easy to regenerate
To ensure that the sanity test fails if doctoc is not properly regenerated.

It is a great idea. I tracked it in #1941 and assigned it to me. I will do it asap :-) but I do not think that it is a blocker for this PR WDYT?

@joelanford
Copy link
Member

I'd rather not add the doctoc without the tooling to generate it, since we'd be asking anyone contributing to documents to figure the tooling out and manually fix things in the meantime.

Can we remove the doctoc from this PR and add it when we add the tooling?

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 19, 2019
Makefile Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 changed the title apply the same standard for all proposals Add automatically index generation + sanity validation + same standard for all proposals Sep 19, 2019
@camilamacedo86 camilamacedo86 changed the title Add automatically index generation + sanity validation + same standard for all proposals Add automatically index generation + same standard for all proposals Sep 19, 2019
@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Sep 19, 2019

Hi @joelanford,

All files are updated and the target was added in the makefile. However, to add in the sanity, will be required do a PR for ci prow repo first for the image used here have the nodejs installed (yum install nodejs).

IMHO: The index is very helpful to check the docs and in fact, we are using doctoc in few of them already and has other ones where the index was added manually which makes very hard keep them maintained. Just have the possibility to update and add them automatically in POV is enough for now. Also, all indexes have a comment with the link for the lib used and the info that they are generated automatically and all is very easy and intuitive to get working.

Please, feel free to review and let me know if we can move forward with or if you would like to change something.

doc/proposals/ansible-operator-status.md Outdated Show resolved Hide resolved
@@ -1,13 +1,22 @@
## <Title of Proposal>
Copy link
Member

Choose a reason for hiding this comment

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

We should discuss as a team, but I wonder if we should start following the openshift/enhancement template for all SDK proposals, and make that the standard going forward? We will likely be using that openshift/enhancement template for a large portion of our future proposals anyway, so perhaps its best if we use it across the board for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really agree with. really makes sense we try to adopt/use the openshift template.

@camilamacedo86 camilamacedo86 force-pushed the ISSU_1910 branch 2 times, most recently from 7c7be32 to eec3242 Compare October 8, 2019 21:23
@camilamacedo86
Copy link
Contributor Author

Hi @joelanford,

I think it is ok to be merged now and it shows enough for we close #1910.

We could here update the current status of the repo proposals
Add the OCP template to be used for the next ones
Perform few adjust in the old ones for them have the same old layout applied

WDYT?

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.

One final comment about the TLS proposal status. LGTM once that's addressed. Great job @camilamacedo86!

doc/proposals/tls-utilities.md Show resolved Hide resolved
@camilamacedo86 camilamacedo86 force-pushed the ISSU_1910 branch 2 times, most recently from 661fdb3 to 30a43d3 Compare October 8, 2019 21:59
@camilamacedo86
Copy link
Contributor Author

/test e2e-aws-ansible

@camilamacedo86 camilamacedo86 merged commit fcc715d into operator-framework:master Oct 9, 2019
@camilamacedo86 camilamacedo86 deleted the ISSU_1910 branch October 9, 2019 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposals should be marked as 'closed' or have some other prominent status
4 participants