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

feat: allow setup for go-vet, govulncheck, and go-build #270

Merged
merged 2 commits into from
May 19, 2024
Merged

Conversation

CallumNZ
Copy link
Contributor

@CallumNZ CallumNZ commented May 15, 2024

Reference: https://github.com/GeoNet/tickets/issues/15422

In doing the above ticket^, I have found the reusable-go-apps workflow not very reusable at all. This change will align it more closely with its name.

Tested here, works: https://github.com/GeoNet/4D-Ashfall/actions/runs/9120866234/job/25079049471

@CallumNZ CallumNZ force-pushed the goAppSetups branch 3 times, most recently from 00d244d to 430006f Compare May 15, 2024 03:04
@CallumNZ CallumNZ requested a review from Mossman1215 May 15, 2024 03:05
Copy link
Contributor

@wilsonjord wilsonjord left a comment

Choose a reason for hiding this comment

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

question: what cases would setups be different for build, vet, and vuln? I'm wondering if buildSetup could simply be used for all three workflows...

@CallumNZ
Copy link
Contributor Author

CallumNZ commented May 15, 2024

@wilsonjord yes, I was wondering the same thing.

build,vet, and vuln would (or more accurately, should) always be the same.
test would be the same, but can often have extra things, so it makes sense to keep this separate.
fmt and imports don't care about the environment.

I would say lint would also be in the first group, but perhaps there's been a situation where it's not?
EDIT: we have never used lintSetup, so am removing it and feeding the buildSetup inputs to it instead.

In practice, these are all the same (at least from what I've seen so far. golangciSetup has never been used in the GeoNet codebase, so not worried about the breaking change.

An attempt to make the reusable-go-apps workflow actually reusuable.
@ardrigh ardrigh requested a review from ozym May 16, 2024 07:54
@ozym
Copy link
Contributor

ozym commented May 16, 2024

Failed the commit conformance. It's quite strict.

Copy link
Contributor

@wilsonjord wilsonjord left a comment

Choose a reason for hiding this comment

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

Perhaps go-test should be ${{ inputs.testSetup || inputs.buildSetup }}

type: string
required: false
description: |
shell commands to setup the test environment
golangciSetup:
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: I don't know if anyone uses this, but if they did, this is a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked, no one uses it.

For the reusable-go-apps workflow
@CallumNZ CallumNZ requested a review from wilsonjord May 19, 2024 20:54
Copy link
Contributor

@wilsonjord wilsonjord left a comment

Choose a reason for hiding this comment

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

lgtm

@CallumNZ CallumNZ merged commit 2877805 into main May 19, 2024
16 checks passed
@CallumNZ CallumNZ deleted the goAppSetups branch May 19, 2024 21:15
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