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

Indicate generated yaml by .gitignore-ing it and including a comment indicating it's generated (to reduce maintenance effort for contributors) #299

Closed
holly-cummins opened this issue Jun 9, 2023 · 23 comments
Assignees
Labels
enhancement New feature or request

Comments

@holly-cummins
Copy link
Collaborator

holly-cummins commented Jun 9, 2023

See discussion.

I asked "do you think it's possible to reduce the volume of docker compose files? I was a bit perplexed by the relationship between rest-fights/deploy/docker-compose/native.yml and rest-fights/src/main/docker-compose/native.yml, for example. One seems to be generated and the other is hand-maintained? For the UI project they're basically identical, but for others there's overlap, but also different content. I couldn't figure out the intention from a search of the readmes. (And a readme probably isn't the right place for that kind of information, because people might not be able to find it mixed in with all the other content, and when they need is when they're looking at the file, trying to understand if it's ok to update or if it's generated.)"

@edeandrea answered:

"For the docker compose stuff, yes src/main are hand maintained "snippets" whereas in the deploy directory is the "real" docker compose file. It's done this way so that we can have the root /deploy/docker-compose with an uber compose file that had everything from all the apps in it."

"It was done that way to try and mimic the kubernetes extension. If we did away with that we'd have to hand craft 2 sets of compose files - 1 individually for each app and 1 uber compose file, both for jvm (and perhaps multiple jvm versions) and native."

@holly-cummins holly-cummins added the enhancement New feature or request label Jun 9, 2023
@holly-cummins
Copy link
Collaborator Author

Thanks, @edeandrea. Is this an accurate picture, or is there stuff in deploy/docker-compose that doesn't go into src/main/docker-compose? For the left side, I've only shown one service, and I've skipped the native/java aspect.

image

@edeandrea
Copy link
Collaborator

Not quite. The deploy/docker-compose is entirely generated by a script as well.

  • app/src/main/docker-compose/infra.yml contains any infrastructure "stuff" that the app needs, but on its own is not a valid docker compose file. It only has the snippets containing the service definitions. Kind of like whats in the files within app/src/main/kubernetes
  • app/src/main/docker-compose/java17.yml (and native.yml) - this is the docker compose file for the app itself. Again, not a valid docker compose file, as its only the snippets containing the service definitions. In comparison with the kubernetes stuff, this is the stuff that the kubernetes extensions would generate, but since we have no docker compose extension, we have to hand-craft

The generate-docker-compose-resources.sh script then goes through app/src/main/docker-compose for each type (i.e. java17, native), combines whats in infra.yml with the java17.yml or native.yml file and creates a valid docker compose file that lives in app/deploy/docker-compose (as either java17.yml or native.yml.

The script also then appends the services to the root /deploy/docker-compose directory into the appropriate file (java17.yml, native.yml).

Does that make sense?

I've also thought about building a docker compose extension that could re-use Dekorate to be able to generate the app specific docker compose files like it does for kubernetes, but I never invested in that (no time unfortunately) and wasn't sure if there are others that would benefit from it either.

@holly-cummins
Copy link
Collaborator Author

Not quite. The deploy/docker-compose is entirely generated by a script as well.

Ah, ok. So the yellow line I proposed is there, but going in the opposite direction?
Actually, I see it's the same direction, but I have the labels on the two files backwards. I was getting quite muddled between the two, trying to remember which path was what.

We should perhaps do the same thing I suggested elsewhere, and .gitignore the generated content, and have the script that generates it run in the CI. And also put a comment in the file explaining that it's generated. Hopefully between those three, it might stop people (like me :) ) investing in maintaining those files.

The ideal would be not even having them in source control, but that would need an extension, as you say.

The generate-docker-compose-resources.sh script then goes through app/src/main/docker-compose for each type (i.e. java17, native), combines whats in infra.yml with the java17.yml or native.yml file and creates a valid docker compose file that lives in app/deploy/docker-compose (as either java17.yml or native.yml.

The script also then appends the services to the root /deploy/docker-compose directory into the appropriate file (java17.yml, native.yml).

Does that make sense?

It does. When I was working through the generate-docker-compose-resources.sh script I spotted the bit that created the aggregated files, and the bit that created the downstream files, but not the bit that created the per-project files. (I'd already manually updated them, so there was no change.)

So I think then there's no more scripting needed, it's just a question of having the generated files be more clearly labelled as generated, so people don't spend time changing them. The comment has to be in the file itself, because we can't make people cross-reference files they might change against readmes to see if the file is listed in the readme as a generated file.

Even this would do the trick, I think, but CI + .gitignore would be more complete:

  touch $output_file
  echo '# this file is generated by generate-docker-compose-resources.sh. do not change it!' >> $output_file
  echo 'version: "3"' >> $output_file
  echo 'services:' >> $output_file

@holly-cummins
Copy link
Collaborator Author

... and ideally also a similar comment in the 'donor' yaml files explaining where that content ends up, and the script strips out that comment ... but that's slightly less trivial to implement.

@edeandrea
Copy link
Collaborator

How would you envision the .gitignore thing actually working? The CI process would have to make multiple commits...one which eliminates the .gitgnore and commits the files, then another commit that reinstantes the .gitignore? Or is there another way?

@holly-cummins
Copy link
Collaborator Author

I'm pretty sure the CI doesn't actually need to commit the change to the .gitignore, it just has to have it locally. So I think something like

echo whatever >> interesting.file
rm .gitignore
git add interesting.file # being specific, here
git commit -m "this only changes the interesting file"
git push

would work. It leaves the CI with a dirty git workspace, but we don't care, because it's an ephemeral machine.

@edeandrea
Copy link
Collaborator

Thats definitely an interesting idea. Worth trying out I think.

@holly-cummins
Copy link
Collaborator Author

It ends up being "one rule for the CI, one rule for the humans," which is slightly confusing, but in combination with a comment on the file is probably clearest overall.

@edeandrea
Copy link
Collaborator

Although it still wouldn't have kept you from modifying the files in the first place. Just would have made you scratch your head as to why your changes didn't commit :)

@edeandrea
Copy link
Collaborator

Although maybe the folder showing up as a different color in the IDE may have given you a clue

@holly-cummins
Copy link
Collaborator Author

Yes, you'd need the comment on the files themselves to alert people to the fact that hand-editing them is (a) ineffectual and (b) a waste of time. :)

@edeandrea
Copy link
Collaborator

I'll also have to check because we are not manually doing git add/git commit/git push. We are using a GitHub action to do it...Which may be fine, but need to investigate.

- name: Commit generated resources
uses: EndBug/add-and-commit@v9
with:
default_author: github_actions
message: "Generate deploy resources (from ${{ github.workflow }} run # ${{ github.run_number }})"
add: '["**/deploy/k8s/*.yml", "deploy/k8s/*.yml", "**/deploy/docker-compose/*.yml", "deploy/docker-compose/*.yml", "deploy/db-init/**"]'
pathspec_error_handling: exitImmediately
new_branch: ${{ env.BRANCH }}

@holly-cummins
Copy link
Collaborator Author

See also quarkusio/quarkus#34025 for the verbosity of the config. I'll update this WI title to keep the scope just at the handling of the generated files.

@holly-cummins holly-cummins changed the title Reduce volume of yaml to make maintenance easier Indicate generated yaml by .gitignore-ing it and including a comment indicating it's generated (to reduce maintenance effort for contributors) Jun 13, 2023
@edeandrea
Copy link
Collaborator

Hey @holly-cummins I was circling back around to this and I have it mostly working in a test repo.

The files look ignored in the IDE, then CI regenerates them, deletes the .gitignore, then commits the generated resources back into source control, leaving the dirty .gitignore alone.

See this commit for an example.

Then in my IDE I see the updates and I can pull them in. Yay!

image

BUT......

What I'm seeing after I pull in the updated files is that the IDE doesn't seem to treat them as ignored anymore, even though the .gitignore file is still the way it should be. I hand-edited one of the generated files and it shows up as changed in the git view (see screenshots below).

image

image

Have you come across anything like this before?

@edeandrea
Copy link
Collaborator

Actually upon further investigation, it doesn't seem that its the IDE that is the problem. If I do a git status I see that the file is modified too..

╰─ git status
On branch main
Your branch is up to date with 'origin/main'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   rest-villains/deploy/docker-compose/java17.yml

no changes added to commit (use "git add" and/or "git commit -a")

@edeandrea
Copy link
Collaborator

It seems that .gitignore is not used when selecting files to commit, it's used for adding only: files listed there are not suggested to be added. But, once they are added (i.e. via a commit once the local .gitignore is removed), they appear in git status.

@edeandrea
Copy link
Collaborator

edeandrea commented Jul 31, 2023

So I'm not sure what we are trying to achieve is something that is technically possible.

I can definitely add something like this to the top of all generated files:

#####################################################################
# THIS FILE IS AUTOMATICALLY GENERATED DURING CI/CD.
# ANY LOCAL CHANGES YOU MAKE SHOULD NOT BE COMMITTED TO SOURCE CONTROL.
#####################################################################

There are ways to tell git locally to ignore local changes to tracked files by doing something like

git update-index --assume-unchanged <filename>

but then any developer would have to do that on their local machine, which I think is a road we do not want to go down.

There are actually 2 options that could get around this a bit: https://www.baeldung.com/git-assume-unchanged-skip-worktree

But again, each user would have to do it for themselves because it is working at the local staging area. And then if you were to git pull changes, it would revert.

@edeandrea edeandrea self-assigned this Jul 31, 2023
@holly-cummins
Copy link
Collaborator Author

Hmm, annoying.

I agree that no matter what, we should add the comment to the generated files.

I guess the other option is that we don't have the CI commit the generated files. If we think people will need them when deploying locally, we could move the generation process to happen in a build step, rather than the CI.

@edeandrea
Copy link
Collaborator

edeandrea commented Jul 31, 2023

I guess the other option is that we don't have the CI commit the generated files. If we think people will need them when deploying locally, we could move the generation process to happen in a build step, rather than the CI.

I guess I don't understand what you mean here? The files are there so it is stupid simple to docker compose up or kubectl apply -f the entire system. We can't assume anyone who wants to deploy the system even has a java runtime installed.

In the case of kubectl apply -f, the user doesn't even need to have the repo cloned locally. They can do something like kubectl apply -f https://raw.githubusercontent.com/quarkusio/quarkus-super-heroes/main/deploy/k8s/java17-kubernetes.yml

@edeandrea
Copy link
Collaborator

I just merged #316 which adds the comment to the top of the generated headers.

@edeandrea
Copy link
Collaborator

One thing we could do, which would involve the user actually doing something, is that we could create a script which "untracks" those files. We could include instructions for users on how to register it as a pre-commit hook in their local repo.

But we couldn't control whether or not they actually did that.

@holly-cummins
Copy link
Collaborator Author

I think asking users to run a script undoes the aim of the changes, which is to try and ensure users can 'do the right thing' in the project without having to read lots of documentation.

So I think we should leave it with just the comment in the files.

@edeandrea
Copy link
Collaborator

I agree. I tend to shy away from anything that would make the user have to do anything else (for which they would need to read something that they already aren't :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants