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

chore: remove build steps from readme and reference build.yml #9558

Merged
merged 3 commits into from Apr 9, 2024

Conversation

mattkrick
Copy link
Member

Description

The best readme is self-documenting code!
Our build.yml script already outlines exactly how to build our app, so our readme should instruct folks to read the yml instead of creating a second source of truth.

I also renamed .yaml to .yml so all our yml files have the same extension.

Signed-off-by: Matt Krick <matt.krick@gmail.com>
@mattkrick
Copy link
Member Author

@rafaelromcar-parabol whatcha think? safe to reference the build yml instead of writing it out?

@rafaelromcar-parabol
Copy link
Contributor

Sorry man. I'm into other things and didn't have time to this.

TBH, I do not like this change. I've used that readme many times to quickly copy and past the commands to build the application locally into a Docker container. I know @Dschoordsch has built it to, but I'm not sure if using this readme.

I agree that removing the readme makes it better in some way, but harder in other. And I like to be able to copy and past if I need to quickly build a Docker image because something is wrong.

I'll like to get @Dschoordsch and @dbumblis-parabol take on this.

@mattkrick
Copy link
Member Author

Ah, I guess I don't know enough about how yall are using this!
@Dschoordsch What's the usecase for building locally? I didn't know devs would ever need to build the app. If we do build, I assume we'd want to make sure we build in the same environment every time, so we'd just let CI do its thing!

@Dschoordsch
Copy link
Contributor

@mattkrick I only build the docker image locally if I want to test/debug something related to the docker image.
@rafaelromcar-parabol A link to the build file is enough for me, I can copy & paste from there.

@rafaelromcar-parabol
Copy link
Contributor

Just to be clear, all the commands to build the application are NOT just like that on the build CI. You have to be able to understand how to run the databases locally using Docker. Not that hard, but just to be clear about it.

@mattkrick
Copy link
Member Author

I think building locally isn't something we should support . We can't guarantee the same architecture, it's easy to have modified node_modules, different env vars, etc. If we want to build an ad-hoc image, we can do that using a GH runner using a workflow:
Screenshot 2024-04-02 at 8 45 20 AM

right now we've got 2 sources of truth on how to build the app: 1 yml, 1 readme.
what i'd propose is we get rid of the readme & instead, if someone really wants to build locally they can use Act to run our github action locally. that way they don't have to copy & paste a bunch of steps!

my goal here is to reduce maintenance overhead. maintaining a readme sucks, so readmes should only cover large fundamental ideas & leave the specifics for the code. whatcha think?

@rafaelromcar-parabol
Copy link
Contributor

It is true that now with the workflow we can build images on demand from branches and just pull them from our private dev repository. I didn't know about Act.

I still think this readme is useful but I understand and agree with what you say about only one source of truth. Let's get rid of it. If we feel the need in the future, we can always add something back.

@mattkrick mattkrick merged commit 0e06d1f into master Apr 9, 2024
5 checks passed
@mattkrick mattkrick deleted the chore/readme-ubi branch April 9, 2024 20:18
@github-actions github-actions bot mentioned this pull request Apr 10, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants