Skip to content

Conversation

miparnisari
Copy link
Contributor

@miparnisari miparnisari commented Jun 13, 2025

Description

  • General fixes to the Github actions

Testing

Checkout the PR locally and ensure all the commands run successfully.

@miparnisari miparnisari force-pushed the remove-swagger-generation branch from 4d72763 to 27750ce Compare June 13, 2025 17:47
@miparnisari miparnisari marked this pull request as ready for review June 13, 2025 17:50
@miparnisari miparnisari requested a review from a team as a code owner June 13, 2025 17:50
@miparnisari miparnisari force-pushed the remove-swagger-generation branch 5 times, most recently from 9ec6880 to 655b50d Compare June 13, 2025 20:06
@miparnisari miparnisari changed the title remove swagger generation fix github actions Jun 13, 2025
@miparnisari miparnisari force-pushed the remove-swagger-generation branch from 655b50d to 1e398c8 Compare June 13, 2025 20:07
Comment on lines +21 to +22
- name: "Lint everything and check for tidy dependencies"
run: "go run magefile.go lint:all && go run magefile.go deps:tidy"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this chained as an && instead of using 2 sequential run operations?

with:
path: ""
fixup-command: "go run magefile.go deps:tidy"
fixup-command: "go run magefile.go lint:all && go run magefile.go deps:tidy"
Copy link
Contributor

Choose a reason for hiding this comment

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

This ran already above, why does it need to run again?

jobs:
go-lint:
name: "Lint Go"
lint:
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing to keep an eye on is that we are now running a more expensive lint job, whereas before we ran multiple concurrent lint operations. This means we are trading off smaller, faster jobs for one larger, more expensive job. That impacts the feedback loop (how long it takes for the developer to receive feedback from CI). This may not be a problem, but please keep an eye on the timing of these jobs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt anyone uses this, but this would be a breaking change, right?

}

// Integration runs the integration tests
func (Test) Integration() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

The unit tests have a timeout of 20m, but the integration tests have a timeout of 1m? No race detector?

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