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

Document ECS RFC process #833

Merged
merged 13 commits into from
Jun 10, 2020
Merged

Document ECS RFC process #833

merged 13 commits into from
Jun 10, 2020

Conversation

epixa
Copy link
Contributor

@epixa epixa commented Apr 30, 2020

I still need to reconcile the existing readme and contributing guide with these changes, but I wanted to put this content into a draft PR for visibility.

The rendered stages doc can be seen here: https://epixa.github.io/ecs/stages.html

@webmat
Copy link
Contributor

webmat commented May 5, 2020

@epixa Thanks for opening this.

What do you think is the best way to host this wide html table?

I think using Github sites as is will lead to unintended complexity. For instance, the main readme can be visualized, the links to other markdown files also work, but the experience is strange. Check out the generated/elasticsearch readme, for example: https://epixa.github.io/ecs/generated/elasticsearch/ :-)

@webmat
Copy link
Contributor

webmat commented May 5, 2020

Note that despite the added scope, I wouldn't mind getting a nice Github website going for ECS. Opens up interesting possibilities.

Also worth pointing out that to generate the stages.html file we could use asciidoc, Github renders it. Note that I'm not suggesting hosting this on the main Elastic docs site, as it's too narrow for the table. I'm just suggesting using Github's asciidoc rendering capabilities if we use Github website.

@epixa
Copy link
Contributor Author

epixa commented May 5, 2020

What do you think is the best way to host this wide html table? I think using Github sites as is will lead to unintended complexity.

I'm open to ideas because I don't love the github pages approach for a single document, but I don't think rendering in the github UI is an option here. The issue is that github enforces a max width on its rendered content that is too narrow for this table. I experimented with making the font size smaller to trade off accessibility for density, but the result was still not great. Manually adjusted column widths helps a bit, but that's not something you can do with generated content on github.

I settled on github pages because it requires no tooling and still uses github as the source of truth, so it seemed like a workable approach with the fewest moving parts.

@epixa epixa force-pushed the master branch 2 times, most recently from 6f2a30c to 9d20085 Compare May 28, 2020 20:42
@epixa
Copy link
Contributor Author

epixa commented May 28, 2020

I've pushed updates that include the RFC template. I ended up going with a single file with stage iterations described in comments. Most of the data is additive anyway, and I think this lends itself to the "living document" notion of the RFC until it is finished.

Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

This is great - the documentation is comprehensive but accessible.

I noted only one open-ended question and a handful of nitpicks.

PROPOSING_CHANGES.md Outdated Show resolved Hide resolved
PROPOSING_CHANGES.md Outdated Show resolved Hide resolved
rfcs/0000-rfc-template.md Outdated Show resolved Hide resolved
rfcs/0000-rfc-template.md Outdated Show resolved Hide resolved
rfcs/README.md Show resolved Hide resolved
epixa and others added 3 commits June 3, 2020 13:57
Co-authored-by: Eric Beahan <ebeahan@gmail.com>
Co-authored-by: Eric Beahan <ebeahan@gmail.com>
Co-authored-by: Eric Beahan <ebeahan@gmail.com>
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

This is looking good. Got a few comments on 2 details.

After those I think we can merge and iterate as we start using this.

rfcs/0000-rfc-template.md Outdated Show resolved Hide resolved
rfcs/0000-rfc-template.md Outdated Show resolved Hide resolved
rfcs/0000-rfc-template.md Outdated Show resolved Hide resolved
rfcs/README.md Outdated Show resolved Hide resolved
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM. Just some redlines from me. Looking forward to seeing this in action.

I think Draft status can be removed given that we are reviewing it 😄 .

PROPOSING_CHANGES.md Outdated Show resolved Hide resolved
PROPOSING_CHANGES.md Outdated Show resolved Hide resolved
PROPOSING_CHANGES.md Outdated Show resolved Hide resolved
PROPOSING_CHANGES.md Outdated Show resolved Hide resolved
rfcs/README.md Outdated Show resolved Hide resolved
@epixa epixa marked this pull request as ready for review June 9, 2020 15:39
@epixa
Copy link
Contributor Author

epixa commented Jun 9, 2020

I pushed changes regarding all outstanding feedback

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

We may have to iterate on making the "stages" table viewable in a sensible way. Did you intend to turn on Github pages for this?

@epixa
Copy link
Contributor Author

epixa commented Jun 9, 2020

@webmat Yep, my plan is to turn on github pages, so the stages doc will be accessible at https://elastic.github.io/ecs/stages.html (linked from PROCESS doc)

@webmat
Copy link
Contributor

webmat commented Jun 9, 2020

Do we need to turn it on for all of Elastic? And if so, should we ring other folks first? ;-)

@epixa
Copy link
Contributor Author

epixa commented Jun 9, 2020

No, it's a per-repo setting. Other projects in Elastic already use github pages, like EUI: https://elastic.github.io/eui/

Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

LGTM

@epixa epixa merged commit 4d472d3 into elastic:master Jun 10, 2020
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