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

Use common workflow #249

Merged
merged 23 commits into from
Aug 17, 2023
Merged

Use common workflow #249

merged 23 commits into from
Aug 17, 2023

Conversation

wenovus
Copy link
Collaborator

@wenovus wenovus commented Aug 7, 2023

No description provided.

@coveralls
Copy link

coveralls commented Aug 9, 2023

Coverage Status

coverage: 84.029%. remained the same when pulling a690305 on use-common-workflow into 23d2b92 on master.

@wenovus wenovus marked this pull request as ready for review August 15, 2023 00:36
@wenovus wenovus requested a review from robshakir August 15, 2023 00:38
Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

Do the linter configs need to be in this repo?

@wenovus
Copy link
Collaborator Author

wenovus commented Aug 15, 2023

Do the linter configs need to be in this repo?

I didn't find a way to have the reusable workflow look at config files outside of this repo. So the only other way is cloning and replacing what's under .github/linters or generating them (if the files don't already exist) during the workflow. Doing this would also require the commit SHA/version of the common-ci repo to be provided as an input argument so that compatible linter configs will be retrieved.

If you look at the other PRs (see list at the description of openconfig/common-ci#2), most other repos use the basic_go.yml workflow which doesn't require any linter config. I converted goyang to this one since go.yml is a bit more comprehensive as an experiment to see how difficult it is to convert.

I wasn't sure whether maintaining this extra complexity was worth it since I'm not certain which of the two basic_go.yml or go.yml we want to use moving forward. basic_go.yml has the advantage of being able to filter out paths very easily as workflow input arguments (by grep -v $(go list ./...)), whereas for golangci-lint you need to add skip-dirs in golangci-lint.yml, which would require maintaining your own version of the linter configs, or providing more tooling to generate these config files that's dependent on the underlying linting tools, which I wasn't sure we wanted to do.

So, the compromise I have is,

  1. Either use basic_go.yml, or
  2. If you want to use more comprehensive checks, use go.yml and maintain your own linter configs.

@robshakir
Copy link
Contributor

Makes sense to me. Thanks for the detailed explanation.

@wenovus wenovus merged commit 08bc968 into master Aug 17, 2023
@wenovus wenovus deleted the use-common-workflow branch August 17, 2023 17:07
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