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

Add YAML Linter #2

Merged
merged 22 commits into from
May 25, 2022
Merged

Add YAML Linter #2

merged 22 commits into from
May 25, 2022

Conversation

parasense
Copy link
Contributor

This was moved here from the previous PR.
The linter is based on, and adapted from, GitHub's "Simple workflow".
I didn't see a YAML linter on the GitHub actions market place portal.

Signed-off-by: Jon Disnard [email protected]

This was moved here from the previous PR.
The linter is based on, and adapted from, GitHub's "Simple workflow".
I didn't see a YAML linter on the GitHub actions market place portal.

Signed-off-by: Jon Disnard <[email protected]>
.github/workflows/lint.yaml Show resolved Hide resolved
.github/workflows/lint.yaml Outdated Show resolved Hide resolved
.github/workflows/lint.yaml Outdated Show resolved Hide resolved
.github/workflows/lint.yaml Outdated Show resolved Hide resolved
@parasense
Copy link
Contributor Author

So I wrote this on my own, it was my first GitHub action, and it's very trivial.
There are others on the GitHub actions marketplace:
https://github.com/marketplace?type=actions&query=yamllint

I would argue this one is as good as any other, and in the future I'm probably going to extend this one to feature schema validation for targeting Tekton bundle contracts:
https://tekton.dev/docs/pipelines/tekton-bundle-contracts/

So I think we should just use this, and see where it goes.

@davidmogar
Copy link
Collaborator

I would argue this one is as good as any other, and in the future I'm probably going to extend this one to feature schema validation for targeting Tekton bundle contracts: https://tekton.dev/docs/pipelines/tekton-bundle-contracts/

So I think we should just use this, and see where it goes.

I disagree. We shouldn’t maintain anything that is already out there. If I’m the future we add something for Tekton schema validation, we will try to find anything existing or try to do it in a way we consume as much from Tekton as possible.

* eliminate push events, we will probably only use PRs.
* make pull-request event types explicit.
* eliminate whitespace

Signed-off-by: Jon Disnard <[email protected]>
@parasense
Copy link
Contributor Author

I disagree. We shouldn’t maintain anything that is already out there. If I’m the future we add something for Tekton schema validation, we will try to find anything existing or try to do it in a way we consume as much from Tekton as possible.

Normally, I would agree 99%. However, this is the 1% where the nuances matter.
Here is what the action would look like if we used somebody else's yaml linter.

name: Lint
on: [push, pull_request]
jobs:
  yamllint:
    name: yamllint
    runs-on: ubuntu-latest
    steps:
      - name: ⤵️ Check out code from GitHub
        uses: actions/checkout@v2
      - name: 🚀 Run yamllint
        uses: frenck/action-yamllint@v1

That's 11 lines of action script, taken from here: https://github.com/frenck/action-yamllint

However, our linter currently looks like this:

name: Linters
on:  # yamllint disable-line rule:truthy
  pull_request:
    branches: ['main']
    types: ['opened', 'reopened', 'synchronize']
  workflow_dispatch:
jobs:
  yamllint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Install yamllint
        run: sudo apt-get install yamllint
      - name: Run yamllint
        run: yamllint --format github $GITHUB_WORKSPACE

It's effectively the same amount of lines and effort. By using somebody else's linter we would gain the advantage of subtracting about ~two lines, and in my humble opinion that is not very persuasive. However, I think your point is valid about maintaining code, less is more... and if this action does grow it makes sense to consolodate.

So after some consideration, I've decided to maintain this linter. I'll post it to the github actions market place using either in my personal profile, or (hopefully) hosted on Red Hat's action repo: https://github.com/orgs/redhat-actions/repositories?q=&type=all

For the time being I think it's reasonable to move forward.

@davidmogar
Copy link
Collaborator

Normally, I would agree 99%. However, this is the 1% where the nuances matter. Here is what the action would look like if we used somebody else's yaml linter.

It's effectively the same amount of lines and effort. By using somebody else's linter we would gain the advantage of subtracting about ~two lines, and in my humble opinion that is not very persuasive. However, I think your point is valid about maintaining code, less is more... and if this action does grow it makes sense to consolidate.

I still disagree.

You would also gain the advantage of not having to maintain anything. Let's say that the package name changes in the future or there's a better package for this. The maintainers of the action will take care of it and you will have to do nothing. And they will probably do that in a way that nothing breaks for you as GitHub actions maintain at least two ubuntu image versions, so they can work in advance.

On top of that, the existing action already has functionality that yours doesn't have. That is strict mode, config files, formatting...

I don't see a need to reinvent the wheel.

Regarding Tekton resources validation, that is not that simple. Usually you would use something like kubeval. The problem with this approach is that it doesn't validate CRD, although I'm quite sure something like this would allow us to validate CRDs. Now, the real problem is that Tekton CRDs don't seem to have the whole spec schema, so validations would always work.

So, let's put this aside and focus on pushing the bundles.

Signed-off-by: Jon Disnard <[email protected]>
Update hello-world accordingly
AMENDED: leaving linted YAML, moving linter action it's own PR.

Signed-off-by: Jon Disnard <[email protected]>
Signed-off-by: Jon Disnard <[email protected]>
* each dirname is the bundle/quay-repo name.
* inside bundle dirs is where pipeline/task yaml's are stored
* This pushes each bundle to quay.io, repos must be created ahead.
* Quay robot/token setup on GitHub secrets config page.

Signed-off-by: Jon Disnard <[email protected]>
* set fetch depth=2 so we can diff previous commit
* look at the diff to see changed files
* only process bundle dirs that had changed files
* drop `kubectl` download, it's not required
* simplify the fake `.kube/conf` to bare minimum

Signed-off-by: Jon Disnard <[email protected]>
* Move bundles into definitions directory
* Various scripting clean-ups
* Sprinked a few printf's to provide workflow feedback.

Signed-off-by: Jon Disnard <[email protected]>
* SHA256 string is now 7 chars long instead of 8
* quay namespace is now hardcoded
* prevent consecutive hyphens or underscore chars in the file path.

Signed-off-by: Jon Disnard <[email protected]>
Signed-off-by: Jon Disnard <[email protected]>
Signed-off-by: Jon Disnard <[email protected]>
* Demo workflow

Signed-off-by: Jon Disnard <[email protected]>

* demo

Signed-off-by: Jon Disnard <[email protected]>
* Demo workflow

Signed-off-by: Jon Disnard <[email protected]>

* demo

Signed-off-by: Jon Disnard <[email protected]>

* Demo

Signed-off-by: Jon Disnard <[email protected]>
Signed-off-by: Jon Disnard <[email protected]>
@parasense
Copy link
Contributor Author

See recent commits; It uses somebodies action, and leverages some parameters.

Some basic rules that seem sane to me.

Signed-off-by: Jon Disnard <[email protected]>
.github/workflows/lint.yaml Show resolved Hide resolved
.github/workflows/lint.yaml Outdated Show resolved Hide resolved
.github/workflows/lint.yaml Outdated Show resolved Hide resolved
.github/workflows/tekton_bundle_push.yaml Outdated Show resolved Hide resolved
.github/workflows/tekton_bundle_push.yaml Outdated Show resolved Hide resolved
definitions/hello-release/hello-release.yaml Outdated Show resolved Hide resolved
definitions/hello-world/hello-world.yaml Outdated Show resolved Hide resolved
parasense and others added 2 commits May 23, 2022 12:26
* Github supposedly installs yamllint already into their images.
* even so, the 3rd party action makes sure to install regardless.
* Regardless, there is no good reason to install yamllint.

Signed-off-by: Jon Disnard <[email protected]>
Copy link
Collaborator

@davidmogar davidmogar left a comment

Choose a reason for hiding this comment

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

Looking good to me. Please, don't merge this without squashing.

@parasense
Copy link
Contributor Author

Something seems to be marked as "requested change", but I'm not 100% clear what that means.
Can we please take another look, and hopefully get this approved today if possible?

@johnbieren
Copy link
Collaborator

I don't see anything requesting change. I have the ability to merge and it is green. But, as David said, we don't want to merge this as 22 commits. They need to be squashed

@parasense parasense merged commit 8278517 into konflux-ci:main May 25, 2022
@parasense parasense deleted the jdisnard_feature_2 branch May 25, 2022 12:39
Troy876 pushed a commit to Troy876/release-service-catalog that referenced this pull request Aug 16, 2023
Add basic YAML linting action to workflow

* Move to using an existing action.
* Add yamllint configuration file to repo
* Some basic rules that seem sane to me.

Related: HACBS-430

Signed-off-by: Jon Disnard <[email protected]>
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.

3 participants