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

cli: add container lint #381

Merged
merged 1 commit into from
Jun 3, 2024
Merged

cli: add container lint #381

merged 1 commit into from
Jun 3, 2024

Conversation

prestist
Copy link
Contributor

@prestist prestist commented Mar 7, 2024

Add an entrypoint that basically everyone can start
adding to their builds which performs some basic
static analysis for known problems.


(original description follows)

We know this does not fix #216 yet, we are working this in time-slots as a part of mobbing to address this. In the next update we plan on having the logic to check for one kernel complete. Then we can debate if we want the lint to be in this code base, or if we want it to be moved somewhere else.

Copy link

openshift-ci bot commented Mar 7, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Mostly nits.

lib/src/cli.rs Outdated Show resolved Hide resolved
lib/src/cli.rs Outdated Show resolved Hide resolved
lib/src/cli.rs Outdated Show resolved Hide resolved
lib/src/cli.rs Outdated Show resolved Hide resolved
lib/src/cli.rs Outdated Show resolved Hide resolved
@cgwalters
Copy link
Collaborator

Also it'd be great to have a test for this, which could be done in a variety of ways there's currently 3 distinct frameworks in use here (GHA, tmt, Prow+kola), which I'd like to take down to just the first two probably, except Prow is the most container-native which is what this test would be oriented towards. But basically building a derived image with two kernels and running the lint in it would make sense.

Another longer term thing to consider here is things like machine-readable output for lints; they're like compiler errors, with the same long term need for tooling that can do more to render than just scrape stdout.

@prestist prestist force-pushed the build-lint branch 2 times, most recently from e3cc97a to 9ed6a19 Compare March 14, 2024 18:18
@prestist
Copy link
Contributor Author

Also it'd be great to have a test for this, which could be done in a variety of ways there's currently 3 distinct frameworks in use here (GHA, tmt, Prow+kola), which I'd like to take down to just the first two probably, except Prow is the most container-native which is what this test would be oriented towards. But basically building a derived image with two kernels and running the lint in it would make sense.

Thank you for that idea, we had not even gotten to the testing stage yet, I think this makes sense and we will look into doing this in our next mob session!

Another longer term thing to consider here is things like machine-readable output for lints; they're like compiler errors, with the same long term need for tooling that can do more to render than just scrape stdout.

Ah so similar to how the context.paths work in butane/ignition?

@cgwalters
Copy link
Collaborator

Ah so similar to how the context.paths work in butane/ignition?

I'm not sure, can you give an example? Or do you mean just how Ignition is inherently JSON?

My thought was like https://doc.rust-lang.org/rustc/json.html (could obviously be much, much simpler than that of course).

@cgwalters
Copy link
Collaborator

See #410 for another good lint.

Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I think this is a good start; basically we can polish it up a bit and ship it I think.

lib/src/cli.rs Outdated Show resolved Hide resolved
lib/src/cli.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
lib/src/cli.rs Outdated Show resolved Hide resolved
@cgwalters
Copy link
Collaborator

#439 is perhaps another thing we could try to lint against. (That said I'm increasingly thinking we should just make everything else be usr_t.)

@cgwalters
Copy link
Collaborator

One thing that came up related to #128 is we found out that people are trying to do RUN podman pull <image> - this is another thing that we could at least detect at build time.

@jmarrero
Copy link
Member

jmarrero commented May 3, 2024

What about the land the initial kernel lint and then add the other lints on other PRs?

@prestist
Copy link
Contributor Author

prestist commented May 7, 2024

@jmarrero, I think that makes sense. I dont want to see this outstanding for much longer. That way we add value, and can add more as time allows. I need to rebase this thing before I add the doc's that colin wanted me to create though. After that the pr should be healthy.

@prestist prestist force-pushed the build-lint branch 5 times, most recently from c36de74 to 181411f Compare May 7, 2024 19:37
@HuijingHei
Copy link

Agree with @jmarrero , make the original PR ready and let CI runs, can check if there is anything regression with the change.

@jmarrero
Copy link
Member

@cgwalters how do you feel about this atm?

If you wanted to have the container subcommand I have an alternative here:

jmarrero@a0de33d

that runs via:

bootc container --lint instead of bootc build-lint

Is there one you think is better still, another option you prefer?

Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Thanks so much for working on this!

lib/src/cli.rs Outdated Show resolved Hide resolved
lib/src/cli.rs Outdated Show resolved Hide resolved
lib/src/cli.rs Outdated Show resolved Hide resolved
lib/src/privtests.rs Outdated Show resolved Hide resolved
@cgwalters
Copy link
Collaborator

cgwalters commented May 30, 2024

I went ahead and did some further updates here:

  • Squashed the commits
  • Moved the linting code into its own module
  • Added targeted unit tests in that module
  • Changed our own container build to use it just to dogfood
  • Adjusted the documentation comments and ran cargo xtask man2markdown to generate manpages

WDYT?

Add an entrypoint that basically everyone can start
adding to their builds which performs some basic
static analysis for known problems.

Closes : containers#216

Co-authored-by: Joseph Marrero <jmarrero@redhat.com>
Co-authored-by: Huijing Hei <hhei@redhat.com>
Co-authored-by: Yasmin de Souza <ydesouza@redhat.com>

Signed-off-by: Steven Presti <spresti@redhat.com>
Signed-off-by: Colin Walters <walters@verbum.org>
@prestist prestist marked this pull request as ready for review June 3, 2024 19:25
@prestist
Copy link
Contributor Author

prestist commented Jun 3, 2024

I went ahead and did some further updates here:

* Squashed the commits

* Moved the linting code into its own module

* Added targeted unit tests in that module

* Changed our own container build to use it just to dogfood

* Adjusted the documentation comments and ran `cargo xtask man2markdown` to generate manpages

WDYT?

Im good to merge; Thank you for the assist! and all the help up to this point! LGTM.

@cgwalters cgwalters changed the title WIP: Build lint cli: add container lint Jun 3, 2024
@cgwalters cgwalters merged commit ebe1530 into containers:main Jun 3, 2024
14 of 16 checks passed
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.

Add bootc build commit
4 participants