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

Podman run without build #141

Merged
merged 7 commits into from
Sep 8, 2021
Merged

Conversation

Kritzefitz
Copy link
Contributor

This makes some changes to the Makefile to integrate better with the official images on Docker Hub. The most prominent goal I had in mind, while making these changes, was being able to use make run to set up podman to pull the images from Docker Hub and run them appropriately. As a side feature it is now possible to use the Makefile to build images that can then be pushed to Docker Hub matching the schema for naming and tagging of images.

These are the individual changes done:

  • The functionality to run a grocy as a pod in podman has been split from make build to make run. The main difference is that make run doesn't depend on make manifest. It is mostly intended to be able to run an image without building it beforehand, instead relying on a previously build images or images that can be pulled from a registry. make build is now just a dummy target that depends on manifest and run.
  • An IMAGE_PREFIX can be passed to make to specify an explicit prefix for the image name. The default is set to docker.io/grocy to match the name of the official builds. But if desired you can also specify a completely different image name such as my.awesome/registry.
  • The IMAGE_TAG is now generated using git describe. This has the main advantage that is uses the git tag as the IMAGE_TAG if one is set on the current commit. Commits without a tag are identified in relation to the previous tag and their commit id.
  • The IMAGE_TAG can be explicitly overridden. This is mostly useful for make run to run another tag than git describe determines. But it could also be useful when building images with different tags.

Makefile Outdated
build: pod manifest
build: manifest run

run: pod
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of separating out the run make target 👍

It makes me wonder whether we should also remove run from the list of build dependencies -- what do you reckon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When testing my changes I didn't find build as a combination of manifest and run very useful. Usually I would first run make manifest first to build and only run make run explicitly when I could be reasonably certain that the build image is actually something I want to deploy.

Now that I think about it, having run as a dependency of build even seems kinda dangerous. It means that git checkout <commit-with-broken-settings>; make build is enough to wipe you existing grocy deployment and replace it with broken containers, even though make build looks quite harmless.

So, yeah, I think its a good idea to remove run from the dependencies of build.

On a related note: I recently used my changed version of the Makefile to deploy grocy productively in podman. When doing so I noticed that whenever I ran make run I usually wanted it to just create the containers (i.e. podman create) and not start them immediately, because I usually wanted to configure other things (such as systemd units) first before starting. Maybe we should provide a create target which would use podman create to create the containers. We could then define run with a dependency on create and use podman pod start to start the pod. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kritzefitz Ok, agreed; let's move run separate to the build target.

Regarding a separate create target: that sounds reasonable to me. Would the create target replace and/or include the steps that are currently inside the pod target?

Bubbling the generated container IDs up through the makefile targets could be a little tricky, but there are probably a few approaches available there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would the create target replace and/or include the steps that are currently inside the pod target?

My current plan looks somewhat like this:

create: pod
  podman create <arguments currently in run>
  podman create <arguments currently in run>

run: create
  podman pod start grocy-pod

i.e. the pod target would stay the same and create would include it.

Bubbling the generated container IDs up through the makefile targets could be a little tricky, but there are probably a few approaches available there.

I don't think we need to. podman pod start can start the pod by name without knowing the container ids involved.

Copy link
Contributor

Choose a reason for hiding this comment

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

All looks like a good plan to me 👍

Would you like write access to grocy/grocy-docker alongside these changes? I'm happy to review them, and then you could merge them after that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I implemented the changes as discussed. Heads up: I force pushed my branch, so you might have to do git reset --hard on my branch to get the current version.

Would you like write access to grocy/grocy-docker alongside these changes? I'm happy to review them, and then you could merge them after that.

That probably makes sense. Do you have any set guidelines or workflows I should follows when I get write access?

Copy link
Contributor

Choose a reason for hiding this comment

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

Review
Thanks for the heads-up re: the force-push. 96ce409 is looking good from local testing here 👍

One thought: I wonder whether we should add a stop make target corresponding to the run target.

Guidelines

Do you have any set guidelines or workflows I should follows when I get write access?

The main workflow-related item to understand, I think, is the release process.

Much of this is convention I've copied from elsewhere, and it's open to modification and/or automation:

Performing a release is aided by a GitHub Actions configuration that is triggered when a tagged release is created within this GitHub repository. A successful build pushes the resulting images to Docker Hub with the corresponding tag.

During maintenance and review, each merged pull request should have an associated changelog entry added (usually under an 'Unreleased' section heading with no date -- like this example).

There are a bunch of manual steps to prepare for and perform a release:

Prerelease

  • Check for any updates to the package.json dependencies (currently, the only dep is a vulnerability scanner)
  • Build all of the changes that are pending to be released (using the Makefile and/or docker-compose)
  • Use npm test to perform automated testing (currently a build and then vulnerability scan)
  • Perform some smoke testing; launch the application and ensure that it's possible to login and use the app

Release

  • Replace all instances of the last-released version number (for example, in the docker-compose.yml and package.json files) with the target release version
  • Update the changelog with the target release version and release date
  • Perform a git commit and then a git tag to label the release
  • Push to GitHub, and create a new release based on the commit + tag, using the tag as the release name, and the most recent changelog section as the release description

NB: In practice I usually forget at least a few of these steps during releases, because I haven't formalized them as a checklist and/or automated them all. Issue #109 also tracks a few ideas for areas of release improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thought: I wonder whether we should add a stop make target corresponding to the run target.

That would make sense for symmetry. But I'm not sure if we want to go that route. If we added stop it would merely be an alias for podman pod stop grocy-pod, without adding any functionality on top. So I don't see any value in being able to use make stop over podman pod stop grocy-pod.

@Kritzefitz Kritzefitz force-pushed the podman-run-without-build branch from 3e9f805 to 96ce409 Compare September 3, 2021 16:09
Kritzefitz and others added 6 commits September 3, 2021 21:20
Before this commit `make build` would always build the images
`localhost/frontend` and `localhost/backend`. This commit allows
setting another prefix than `localhost`. The default prefix has been
set to `docker.io/grocy` to get the same image names as the official
images on Docker Hub.
This is useful if you don't want to build the containers yourself and
instead pull the official builds from Docker Hub.
This is useful if you want to pull images that don't match the current
git tag.
git describe incorporates tags into the output to show the relation of
builds to existing releases. It also automatically appends `-dirty` if
appropriate.

This is mostly useful, because it generates tags appropriate for
publishing on Docker Hub if the build is run on a git tag.
`make build` looks like a rather harmless build. Having `run` be a
dependency of `build` means that a harmless `make build` can
potentially destroy a productive setup.
`make create` can be used to create the pod without starting it. `make
run` uses `create` to create a pod and uses `podman pod start` to
start it.
@Kritzefitz Kritzefitz force-pushed the podman-run-without-build branch from 96ce409 to 403bfbf Compare September 3, 2021 19:20
Copy link
Contributor

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @Kritzefitz!

@Kritzefitz Kritzefitz merged commit a502178 into grocy:main Sep 8, 2021
@Kritzefitz Kritzefitz deleted the podman-run-without-build branch September 8, 2021 08:19
@Kritzefitz Kritzefitz mentioned this pull request Sep 8, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants