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

fix(ci): Re-add markdown and cue checks #9285

Merged
merged 2 commits into from
Sep 21, 2021
Merged

Conversation

jszwedko
Copy link
Member

These were (accidentally?) removed in
#7368

Includes markdown fixes.

Closes: #9270

These were (accidentally?) removed in
#7368

Includes markdown fixes.

Closes: #9270

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
@netlify
Copy link

netlify bot commented Sep 21, 2021

✔️ Deploy Preview for vector-project ready!

🔨 Explore the source changes: a24132a

🔍 Inspect the deploy log: https://app.netlify.com/sites/vector-project/deploys/614a07baf04edf000884d8f0

😎 Browse the preview: https://deploy-preview-9285--vector-project.netlify.app

Copy link
Member

@bruceg bruceg left a comment

Choose a reason for hiding this comment

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

Thanks @jszwedko

@@ -6,6 +6,6 @@ layout: component
tags: ["aws", "cloudwatch", "component", "sink", "logs"]
---

{{/* This doc is generated using:
{{/*This doc is generated using:
Copy link
Contributor

Choose a reason for hiding this comment

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

Basically the reason I disabled these is that the Markdown linter doesn't "understand" Go templating, which means that using the linter will invariably force us into awkwardnesses like these. I'm fine with the linter for the RFCs and things like that but for the docs I'd prefer to disable it, as it's simply not syntax aware and I question its value.

Copy link
Contributor

Choose a reason for hiding this comment

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

In particular I'm referring to this:

{{/*This doc is generated using:
     1. The template in layouts/docs/component.html
2. The relevant CUE data in cue/reference/components/...*/}}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is fair. If the linter doesn't gracefully Go templates, then I can see why we wouldn't want to use it for the Hugo markdown files. I'll see if I can fix this particular issue. That seems to me to be the only undesirable change in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed this in a24132a which I personally think results in more readable comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jszwedko Okay, I'm satisfied with that. If more edge cases like this pop up we could maybe disable the linter in a granular way (e.g. exclude website/content/en/docs/reference/configuration/{sinks,sources,transforms}/* but I think we're good for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Yeah, that makes sense, we can definitely keep an eye out for if it starts requiring less readable docs.

@@ -9,6 +9,7 @@ tags: ["aws", "cloudwatch", "logs", "firehose", "advanced", "guides", "guide"]
---

{{< requirement title="Pre-requisites" >}}

Copy link
Contributor

Choose a reason for hiding this comment

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

This is another one of the awkwardnesses I was referring to. To me these empty lines look not awesome.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually like the whitespace here, but honestly it doesn't matter to me as long as it is consistent which is the value that I think a linter provides.

@lucperkins
Copy link
Contributor

As noted in my comments, overall I question the value of the Markdown linter in website/content/*. Removing this linter was deliberate, as I feel like it's going to produce unnecessary consternation (submit PR -> see error -> track down nit -> make change that doesn't seem quite right to satisfy linter).

@spencergilbert
Copy link
Contributor

As noted in my comments, overall I question the value of the Markdown linter in website/content/*. Removing this linter was deliberate, as I feel like it's going to produce unnecessary consternation (submit PR -> see error -> track down nit -> make change that doesn't seem quite right to satisfy linter).

Fair compromise to run them only outside of the website/ dir?

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
@jszwedko
Copy link
Member Author

As noted in my comments, overall I question the value of the Markdown linter in website/content/*. Removing this linter was deliberate, as I feel like it's going to produce unnecessary consternation (submit PR -> see error -> track down nit -> make change that doesn't seem quite right to satisfy linter).

I think consternation is part of the role of a linter 😄 I personally see value in having consistently formatted cue and markdown, even in the website directory. The linter can be run locally when making changes.

We can keep an eye out to see if it starts causing a lot of pain with Go templates though.

@jszwedko jszwedko enabled auto-merge (squash) September 21, 2021 17:02
@jszwedko jszwedko merged commit a6b304d into master Sep 21, 2021
@jszwedko jszwedko deleted the check-markdown-and-cue branch September 21, 2021 17:47
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.

The "check docs" CI step is no longer being run
4 participants