openconfig / goyang

YANG parser and compiler to produce Go language objects
Apache License 2.0
218 stars 83 forks source link

Use common workflow #249

Closed wenovus closed 1 year ago

coveralls commented 1 year ago

Coverage Status

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

wenovus commented 1 year ago

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 https://github.com/openconfig/common-ci/pull/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 commented 1 year ago

Makes sense to me. Thanks for the detailed explanation.