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

CI to vet + build on push #27

Closed
wants to merge 1 commit into from
Closed

CI to vet + build on push #27

wants to merge 1 commit into from

Conversation

abe-winter
Copy link
Member

What changed

  • add workflow on every push which vets and builds agent

Why

Minimal, non-invasive linter coverage. We can improve later as needed.

Copy link
Member

@Otterverse Otterverse left a comment

Choose a reason for hiding this comment

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

So I appreciate the initiative, but I don't see a use case for this in a basic state here. We have a backlog task to do this correctly/thoroughly (including building/publishing releases), but it just hasn't been priority. If some super-basic version of this saves you some time or something though, then happy to consider. But as something totally opt-in, I feel like we get the same from just running make lint locally.

That said, from my PoV on this in general, I think we need this on PRs more than on push, and would need to be minimally a blocker for merges in order to be useful. I'd also want to see it use make lint all for local replication, rather than directly running go commands. I think it'd also need to check/fail if the lint results in any changes, like most of our other repos do.

Oh, and would need to restrict push (if it were to remain, instead of moving to pull_request) to just the main branch. Right now, I push to other branches just to save my work (e.g. when leaving NYC to fly home) and that code isn't always going to compile or lint.

Lastly, whatever gets put in here is probably going to be identically copied to the subsystem repos (agent-provisioning, agent-syscfg) as well. Just to keep in mind (nothing here looks like it wouldn't work there too, so far.)

@abe-winter
Copy link
Member Author

agree with everything you said -- intended this as a 'better than nothing' baseline, not as a complete answer

should I make these changes:

  • restrict on:push to main branch
  • add PR merge blocking

or close for now, wait for complete version

@Otterverse
Copy link
Member

agree with everything you said -- intended this as a 'better than nothing' baseline, not as a complete answer

should I make these changes:

  • restrict on:push to main branch
  • add PR merge blocking

or close for now, wait for complete version

Yeah, totally understand it was just a short "baseline" attempt. Still, I'd vote "close" unless you feel you need/want this for some benefit to your own workflow at the moment. Only you, me, and Ale are in this codebase.

@abe-winter
Copy link
Member Author

closing

@abe-winter abe-winter closed this Jun 11, 2024
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