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

Add CI, Tag, Backport, Commands workflows and issue templates #62

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

ulucinar
Copy link
Contributor

Description of your changes

This PR adds CI, Tag, Backport, Commands workflows and issue templates, and:

  • Moves uptest to cmd/uptest
  • Adds version.Version
  • Adds copyright headers
  • Linter fixes

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

make reviewable

Also the linter fixes on the regular expressions have been manually validated.

@ulucinar ulucinar force-pushed the ci-fix branch 5 times, most recently from 925811e to a7c0bc5 Compare January 15, 2023 23:54
@jeanduplessis
Copy link
Contributor

jeanduplessis commented Jan 16, 2023

@ulucinar can we merge the stuff from #28 in with this PR? I think its time to pull the trigger on that one.

I recommend making the repo naming more generic than crossplane-provider-tools so it can cover all extensions to Crossplane, not just providers. Something like: upbound/extension-tools or similar

@ulucinar
Copy link
Contributor Author

Hi @jeanduplessis,
I'd also like to split uptest and crddiff into separate binaries as we will now have a proper build pipeline for the repo. I suggest continuing with a series of PRs so that reviews are easier, where:

  • We will refactor tools into their own binaries & add the version subcommand
  • Rename the repo
  • There is still some common functionality duplicated in the official provider repos. Refactor the pipelines further and consider moving those (e.g., the CI workflow for official providers).

What do you think?

@jeanduplessis
Copy link
Contributor

@ulucinar a series of PRs sounds good to me 👍🏻

- Move uptest to cmd/uptest
- Add version.Version
- Add copyright headers

Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

A few comments pertaining to linters.

}

func generateRFC1123SubdomainCompatibleString() string {
rand.Seed(time.Now().UnixNano())
s := make([]rune, 8)
for i := range s {
s[i] = charset[rand.Intn(len(charset))]
s[i] = charset[rand.Intn(len(charset))] // #nosec G404: no need for crypto/rand here
Copy link
Member

Choose a reason for hiding this comment

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

Can // nolint cover this for consistency with the other overrides?

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.

@@ -232,7 +231,7 @@ func filterNonBreaking(diffMap map[string]*diff.Diff) map[string]*diff.Diff {
return diffMap
}

func ignoreOptionalNewProperties(sd *diff.SchemaDiff) {
func ignoreOptionalNewProperties(sd *diff.SchemaDiff) { // nolint:gocyclo
Copy link
Member

Choose a reason for hiding this comment

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

I highly recommend enabling nolintlint. It requires folks to explain why they're adding a nolint directive, and also catches nolint directives that are ineffective, e.g. because a function that used to be over the cyclomatic complexity level later dropped below it.

crossplane/crossplane@03609c4

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. Also broken some high cyclomatic complexity functions.

.golangci.yml Outdated
Comment on lines 30 to 32
revive:
# confidence for issues, default is 0.8
confidence: 0.8
Copy link
Member

Choose a reason for hiding this comment

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

I suggest removing this block. Doing so causes revive to catch the things that golint used to catch. See crossplane/crossplane@74e387b for more context.

Copy link
Contributor Author

@ulucinar ulucinar Jan 17, 2023

Choose a reason for hiding this comment

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

Thank you @negz, very helpful. When this configuration block is removed, we now have to document, for example, exported APIs. Removed this block, and added some package docs and exported API docs.

Copy link
Member

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @ulucinar LGTM!

@ulucinar ulucinar merged commit 8da6199 into upbound:main Jan 19, 2023
@ulucinar ulucinar deleted the ci-fix branch January 19, 2023 11:33
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.

4 participants