Skip to content
This repository was archived by the owner on Mar 3, 2025. It is now read-only.

(actions) Add sanity check#29

Merged
anik120 merged 4 commits intooperator-framework:mainfrom
anik120:add-sanity-check
Apr 12, 2023
Merged

(actions) Add sanity check#29
anik120 merged 4 commits intooperator-framework:mainfrom
anik120:add-sanity-check

Conversation

@anik120
Copy link
Copy Markdown
Member

@anik120 anik120 commented Apr 11, 2023

Follow up to #24

everettraven
everettraven approved these changes Apr 11, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Non blocking nit.

/lgtm

Comment thread .github/workflows/sanity.yaml Outdated
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2023
@anik120
Copy link
Copy Markdown
Member Author

anik120 commented Apr 11, 2023

hold.

Looks like it's trying to cd into the wrong directory structure:

(cd /home/runner/work/catalogd/catalogd/hack/tools && go mod tidy)
[142](https://github.com/operator-framework/catalogd/actions/runs/4671233921/jobs/8272096978#step:4:143)
/bin/sh: 1: cd: can't cd to /home/runner/work/catalogd/catalogd/hack/tools

@anik120 anik120 added this to the v0.1.0 milestone Apr 11, 2023
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2023
joelanford
joelanford reviewed Apr 11, 2023
View reviewed changes
Comment thread .github/workflows/sanity.yaml
Copy link
Copy Markdown
Member

@joelanford joelanford Apr 11, 2023

Choose a reason for hiding this comment

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

Nit: rename to test.yaml so we can put all the test-oriented jobs in here?

  • test/sanity
  • test/markdown
  • test/unit
  • test/e2e
  • etc.

Copy link
Copy Markdown
Member Author

@anik120 anik120 Apr 11, 2023

Choose a reason for hiding this comment

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

@joelanford I JUST opened another PR to add a separate action for unit test #31 😄

Copy link
Copy Markdown
Member Author

@anik120 anik120 Apr 11, 2023

Choose a reason for hiding this comment

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

It feels easier to navigate the folder if all of the jobs are laid out in their specific files, but it doesn't seem like a long enough list that it'll be extremely hard to navigate one file. If you feel strongly about putting it all in one file I can change the name of the file.

Copy link
Copy Markdown
Member

@joelanford joelanford Apr 11, 2023

Choose a reason for hiding this comment

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

My thinking is along the lines of this using a matrix where we can essentially template out the workflow steps, like this:

https://github.com/operator-framework/operator-controller/pull/137/files#diff-245392b692a50c38ecab4381b118862db514035c10983f3bd4f4b7f1f4be4692

Copy link
Copy Markdown
Member

@joelanford joelanford Apr 11, 2023

Choose a reason for hiding this comment

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

But this is a nit/something we could do improve later.

joelanford
joelanford reviewed Apr 11, 2023
View reviewed changes
Comment thread .github/workflows/sanity.yaml Outdated
Comment on lines +26 to +27
- name: Lint markdown files
uses: github/super-linter/slim@v4
Copy link
Copy Markdown
Member

@joelanford joelanford Apr 11, 2023

Choose a reason for hiding this comment

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

Is it possible to run this locally? If not, it seems pretty annoying that the only way to know whether markdown lint is going to pass is if you submit a PR or push a branch.

Also (and may this makes my first question moot), should we put this in a separate PR so that we can more easily debate the merits of this test without holding up the make verify check being added to CI.

Copy link
Copy Markdown
Member Author

@anik120 anik120 Apr 11, 2023

Choose a reason for hiding this comment

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

https://github.com/igorshubovych/markdownlint-cli#readme I ran it locally to fix the markdown errors.

Having said that, I can take this to another PR so that sanity is not blocked.

Copy link
Copy Markdown
Collaborator

@everettraven everettraven Apr 11, 2023

Choose a reason for hiding this comment

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

+1 on running locally

Also (and may this makes my first question moot), should we put this in a separate PR so that we can more easily debate the merits of this test without holding up the make verify check being added to CI.

This is a fair point. I noticed that rukpak has this action so I assumed that had already been debated:

Copy link
Copy Markdown
Member

@joelanford joelanford Apr 11, 2023

Choose a reason for hiding this comment

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

Probably done out of expediency and no one noticed/cared. It isn't that big of a deal, tbh, but I just happened to notice. It'd be nice to add that linter as another tool download and then invoke it in a make target like we do other things.

Copy link
Copy Markdown
Member Author

@anik120 anik120 Apr 11, 2023

Choose a reason for hiding this comment

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

I pulled the markdown linter to a separate PR so that we can have this conversation there: #32

joelanford
joelanford reviewed Apr 11, 2023
View reviewed changes
Comment thread README.md Outdated
joelanford
joelanford reviewed Apr 11, 2023
View reviewed changes
Comment thread .github/workflows/sanity.yaml
anik120 added 2 commits April 11, 2023 16:54
@anik120
(actions) add sanity check
06041d2
@anik120
(Makefile) fix target tidy
50d1469 
Tidy was being instructed to `go mod tidy` the hack/tools folder,
which was being done when the tools were being downloaded using
go modules (the main branch has the setup to download the tools
using go installer instead)
@anik120 anik120 mentioned this pull request Apr 11, 2023
Closed
joelanford
joelanford approved these changes Apr 11, 2023
View reviewed changes
Copy link
Copy Markdown
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.

/approve

Can we follow-up on the go mod caching matrix style tests?

@anik120
Copy link
Copy Markdown
Member Author

anik120 commented Apr 11, 2023

Can we follow-up on the go mod caching matrix style tests?

I've added uses: actions/cache@v3 in the job to set up the initial stuff, but we can follow-up in depth in another PR.

@joelanford
Copy link
Copy Markdown
Member

joelanford commented Apr 12, 2023

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2023
@anik120 anik120 merged commit be87782 into operator-framework:main Apr 12, 2023
@everettraven everettraven mentioned this pull request Apr 12, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@everettraven everettraven everettraven approved these changes

@joelanford joelanford joelanford approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@joelanford joelanford

@everettraven everettraven

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

v0.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@anik120 @joelanford @everettraven