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

Actually continuously integrate #1129

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

northeastprince
Copy link
Contributor

This organizes our workflows and adds a custom action to make files more consistent and thus more easily modifiable. I highly recommend this gets merged as a rebase and not a squash so that the formatting changes are separated.

@northeastprince northeastprince requested review from a team as code owners April 9, 2024 20:41
Copy link
Contributor

@grymmy grymmy left a comment

Choose a reason for hiding this comment

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

Seems good to have overall. I wonder slightly if the node_modules directory is necessary to be checked in...?

please shepherd this one carefully to production and test well @northeastprince

@grymmy
Copy link
Contributor

grymmy commented Apr 9, 2024

I guess one other thing is I don't often like CI jobs that modify files + autocommit after the PR is sent, in ci...another option is to use commit-time checks and put the work on the user - but tbh it looks like some thought/work was put in here and it does serve a good purpose

Copy link
Member

@Muirrum Muirrum left a comment

Choose a reason for hiding this comment

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

Please remove node_modules from this commit

@Muirrum
Copy link
Member

Muirrum commented Apr 9, 2024

I agree with Graham that I'm not a fan of CI runs that alter repo state -- is there a need for this task to run in CI, or can CI be a verification check that validates the correct format after the user runs a tool either as a commit hook or as a Makefile-esque task?

@northeastprince
Copy link
Contributor Author

I'm confused, what would that accomplish? Additionally, if you take a look at the GitHub Actions documentation, you'll see that the dependencies have to be there.

@Muirrum
Copy link
Member

Muirrum commented Apr 9, 2024

Huh, I'm not a fan of committing node_modules, but that's what the docs say.

That would accomplish keeping the repo in a "known state" -- having automatic commits made after PRs are merged is non-obvious and may be unexpected, and it leads to a lot of spurious commits as well. Additionally, if the tool makes mistakes, it's easier and cleaner to fix them if they happen in a local working copy, than if they happen in a commit to the main branch.

In general, systems should do the obvious thing. In this case, I would rather put the work on the user to run a formatting script or pre-commit hook to enforce style, and then have the CI run do the integration testing of "is this sorted/formatted properly?". Looking at the logs, this also produces no logging of what gets changed and why, which I would really like to see if this is modifying data.

@northeastprince
Copy link
Contributor Author

I would agree that automatic commits to main are rightfully off-putting, but aside from the initial formatting, this would happen in PRs from now on (due to the way Actions works this needs to be merged first before something actually happens). The additional commit as opposed to amending the previous one seems to be pretty separate, and most PRs are just adding a record (so the changes are gonna be straightforward in basically every one of these runs except the first one). I think it's a pretty simple system, but if for some reason we need to add more complex logic in the future, I'm all for programmatically commenting with the reasons for the changes.

@Muirrum
Copy link
Member

Muirrum commented Apr 9, 2024

I'm against any changes being done without logging of what change is being made and why. If something breaks, being able to go back and see what made the breaking change will be invaluable.

Also -- under no circumstances should we be amending other people's commits programmatically. A commit should always represent a single unit of work that once pushed to a remote, is nominally immutable. You can do whatever you want on your own working copy, and even whatever you want in your own fork, but once it hits a PR or hits the hackclub/dns repo, it shouldn't change

Copy link
Contributor

@reesericci reesericci left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good, just have a few changes:

  • Don't check in node_modules
  • Let's not change people's code for them, have a makefile which runs the formatter and then check it with the action in a PR

Copy link
Contributor

@reesericci reesericci left a comment

Choose a reason for hiding this comment

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

changing to requested changes because github bugged out

@northeastprince
Copy link
Contributor Author

As I've already said, node_modules is required; look at the docs.

I do wonder though: what would change, practically, if someone had to run a script locally to format the files and commit the changes before their PR could be merged? Well, first of all, I'm willing to bet that the overwhelming majority of contributors don't even checkout the repo locally (they just edit it in GH). Which means if they want to get their PR through, they'll have to check it out, install node, make sure the right dependencies are installed too, run the script, and push the changes. Most people won't wanna do that, and rightfully so - it takes up unnecessary time and effort. When a lot of PRs inevitably start becoming stale, maintainers can either close the PR (which is pretty petty) or fix it themselves. Regardless, all possible paths forward have the same benefits that come with consistency. One, however, eliminates the burden of both unnecessary time and effort on both contributors and maintainers and has no cons besides the temporarily uncomfortable nature of change. Our industry had a bad habit of taking extremist positions. We're lucky that Rails embraces the exact opposite; they recognize that, very often in software development, different architectures mandate different approaches - a quilt of patterns, not cult-like paradigms not to be challenged. While complex code can benefit from manual linting, a massive collection of data sharing the exact same format can afford to make different tradeoffs in the name of developer happiness. If I'm missing a point here, feel free to point it out.

@grymmy
Copy link
Contributor

grymmy commented Apr 10, 2024

I'm against any changes being done without logging of what change is being made and why. If something breaks, being able to go back and see what made the breaking change will be invaluable.

Another way of stating this is WYSIWYG is good, this adds unnecessary complexity because it transforms what the author configured into another form, and they will never appreciate that they weren't complying with a convention we require.

Could we use git/github hooks and cause errors on commit using a lint rule? Perhaps something that'd allow for schema validation of sorts (beyond just valid yaml syntax)? TBH even a heuristical sort program such as yours there could be quickly modified to, instead of modifying the config on behalf of the user, simply verify input == output (fail if unsorted) to serve as the validation engine...

@northeastprince
Copy link
Contributor Author

northeastprince commented Apr 10, 2024

I'm still confused as to what that would accomplish besides requiring people to run a script themselves. So far I'm not seeing any specific pitfalls being brought up besides the fact that automatic changes aren't common practice, which I've addressed above. If there aren't any, I'd propose we merge it since we can then discuss its effect in real life and not just vauge hypotheticals.

@grymmy
Copy link
Contributor

grymmy commented Apr 10, 2024

I also kinda call into question this specific feedback as being a maximalist or extremist overreaction or anything of the sort tbh - venturing the conversation there to that vantage point seems a bit... extreme to me 🤣. I think we should focus on discussing the facts, which to me seem to be tradeoffs between ease-of-use and repo hygiene:

  • One stance is that developer experience / ease of just getting things done supercedes the importance of repo hygiene, and it benefits newcomers by lowering the bar thus making dns repo easier to contribute to. It argues that we can use computers to keep it clean and git history - pfft, that can be easily filtered! Plus, it's already implemented! 😎

  • The other is that it is important to keep our git history clean and ensure developers have a clear relationship with the actual code that goes to production, at commit time, w/o transformations. Asking PR authors to alphabetize (or even have deeper schema validations of - this is not something to ignore) their stuff is not a big hill to ask them to climb, especially if that feedback is given very quickly. Simple combo of git hooks (or just gh actions) makes for a reasonable UX, this is pretty common practice, and very few changes would be needed to adapt this diff to that.

I see valid points on both sides. I tend to lean towards "if it works it works, and even better if it is already implemented", but at the same time ppl are gonna run into validation failures regardless (invalid yaml, other deeper schema validations that we might want to do) that we'll demand them fix those anyway...

@northeastprince
Copy link
Contributor Author

I'm fine with using the action to format things initially and changing it after to be just a check, but I'm confused on how it would impact repo hygiene besides the one commit necessary in main for the initial sorting/formatting (unless that's what you're referring to, in which case it'd be necessary anyway with the other option).

@grymmy
Copy link
Contributor

grymmy commented Apr 30, 2024

@northeastprince Do we plan on moving forward with small changes to this PR?

@northeastprince
Copy link
Contributor Author

I was waiting for a response to my comment - sorry if that was unclear. Just to clear things up from what I presume to be the main source of confusion, code and data formats meant to be read and modified by humans are two entirely different things, so I think we can afford to try a different approach. I'm still confused on what is actually meant by the broad strokes of concern I've seen like "hygiene", as I assume by now if specific things came to mind they would be brought up. If there are none, I don't see the harm in merging.

@grymmy
Copy link
Contributor

grymmy commented Apr 30, 2024

I was waiting for a response to my comment - sorry if that was unclear. Just to clear things up from what I presume to be the main source of confusion, code and data formats meant to be read and modified by humans are two entirely different things, so I think we can afford to try a different approach. I'm still confused on what is actually meant by the broad strokes of concern I've seen like "hygiene", as I assume by now if specific things came to mind they would be brought up. If there are none, I don't see the harm in merging.

I think specific things were brought up re: the github action programmatically modifying the git record - changing human-written code - that were not addressed, and I think is the major point of contention re: this diff landing. This change would potentially complicate things like reverting changes easily and cleanly, for instance - which is one argument one can make for hygiene being an issue here. Moreso, it is done without developers' knowledge for the most part - unless they are aware of this action. This can cause confusion and chaos when trying to understand what might be happening in production environments. My suggestion to you is to directly address these concerns with counter-arguments that justify your approach and convince those that might be counter to your PR (I'm on the fence myself, I could lean either way) that it's worth it regardless.

@grymmy
Copy link
Contributor

grymmy commented Apr 30, 2024

Either that, or resolve the commentary with changes to the PR.

@northeastprince
Copy link
Contributor Author

It seems like the automated commit would impact a very small percentage of PRs (ones without a single straightforward commit), but combined with the fact that it would be an unorthodox approach and appear confusing, I propose a command that runs the action and a check to make sure no PR is merged without running it if needed.

@Muirrum
Copy link
Member

Muirrum commented May 10, 2024 via email

@northeastprince northeastprince requested a review from grymmy May 10, 2024 21:14
@jaspermayone jaspermayone requested review from jaspermayone, Muirrum and a team May 15, 2024 20:33
@jaspermayone
Copy link
Contributor

Note on this PR, I made changes on main to some names that will need to be updated here.

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.

5 participants