Skip to content

Conversation

@apostasie
Copy link
Contributor

@apostasie apostasie commented Feb 23, 2025

This PR cleans-up the Makefile:

  • reorganize variables
  • separate fix-* and lint-* tasks
  • provide and emphasize use of simple one-stop high-level tasks (fix, lint, test)
  • add a task for the developer to install vetted dev tools easily (install-dev-tools)
  • cleaned-up github workflows a bit, accordingly
  • prettify the make tasks display
  • parameterize nerdctl binary name to $(BINARY)
  • parameterize $(ORG_PREFIXES) consistently
  • add additional lint tasks to perform verifications that are handled by project-checks on the CI
  • cleanup PHONY
  • minor additions to .golangci to simplify command-line invocation of golangci-lint and make the output more comprehensive and easier to grok

Note that this should be merged after #3911 as it duplicate the changes from there.

@apostasie
Copy link
Contributor Author

Also #3915 should be merged first.
I will rebase as necessary.

@apostasie apostasie force-pushed the ci-make-tasks branch 2 times, most recently from ddf9b0c to 7308908 Compare February 23, 2025 22:16
@apostasie
Copy link
Contributor Author

CI failures are unrelated IPFS/Compose and github cache sluggishness timeouting.

@apostasie
Copy link
Contributor Author

Failures are all timeouts from slow github cache / build.

@apostasie
Copy link
Contributor Author

Rebased.

@apostasie apostasie closed this Feb 25, 2025
@apostasie apostasie reopened this Feb 25, 2025
@apostasie apostasie closed this Feb 25, 2025
@apostasie apostasie reopened this Feb 25, 2025
@AkihiroSuda AkihiroSuda added this to the v2.0.4 milestone Feb 25, 2025
- unnecessaryBlock

issues:
max-issues-per-linter: 0
Copy link
Member

@AkihiroSuda AkihiroSuda Feb 26, 2025

Choose a reason for hiding this comment

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

Why do we need this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that golangci does not strip the output and show all the errors instead.
It is frustrating to run golangci, fix the issues, then have to run it again for "more" issues.

wdyt?

VERSION ?= $(shell git -C $(MAKEFILE_DIR) describe --match 'v[0-9]*' --dirty='.m' --always --tags)
VERSION_TRIMMED := $(VERSION:v%=%)
REVISION ?= $(shell git -C $(MAKEFILE_DIR) rev-parse HEAD)$(shell if ! git -C $(MAKEFILE_DIR) diff --no-ext-diff --quiet --exit-code; then echo .m; fi)
LINT_COMMIT_RANGE ?= main..HEAD
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to work for release/X.X branches, which are created when we are not ready to make a release from the main branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm. Let me test that immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you do not want the range compared to main:

git checkout origin/release/1.7
touch foo; git add foo; git commit -S -s -m "Foo"
LINT_COMMIT_RANGE=origin/release/1.7..HEAD make lint-commits
INFO[0000] using commit range: origin/release/1.7..HEAD
 * ff0ac028 "Foo" ... PASS

Can you let me know what use-case you would like?

Copy link
Member

Choose a reason for hiding this comment

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

Overriding LINT_COMMIT_RANGE seems good.
This should be set in the GHA yaml with $GITHUB_BASE_REF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Let me look at the workflow files right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refreshing myself on this: we do not run on the CI make lint-commits (or the overarching make lint).

The reason is that project-checks does do the git-validation for us already (and that would just duplicate the check).

I am going to add a note to the md file about proper local use of make lint-commits though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added a few words in the testing markdown doc about this specific point.

Let me know your thoughts.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda
Copy link
Member

Needs rebase

Signed-off-by: apostasie <spam_blackhole@farcloser.world>
@apostasie
Copy link
Contributor Author

Rebased. Can you poke the failed run?

@AkihiroSuda AkihiroSuda merged commit 1fd291f into containerd:main Feb 27, 2025
30 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.

2 participants