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.

Sign up for GitHub

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

Jump to bottom

Add Dockerfile pt. 2 #124

Merged
merged 13 commits into from
Jul 10, 2021
Merged

Add Dockerfile pt. 2 #124

merged 13 commits into from
Jul 10, 2021

Conversation

Samyak2
Copy link
Member

@Samyak2 Samyak2 commented Jun 5, 2021

Description

Fixes #101

Based on #106

  • Added a Dockerfile based on the golang image
  • grofer is installed using go install from the local source
  • The whole root directory of the host has to be mounted as read only for this to work. A few other docker flags are also required.
    • the docker socket is also mounted
  • See updated README for instructions
  • HOST_PROC, HOST_SYS, etc. is set for gopsutil

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • I have read the contribution guidelines and followed it as far as possible.
  • I have performed a self-review of my own code (if applicable)
  • I have commented my code, particularly in hard-to-understand areas (if applicable)
  • I have run go fmt on my code (reference)
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • Any dependent and pending changes have been merged and published

TODO

  • fix GitHub workflow to publish container

@Samyak2
Copy link
Member Author

Samyak2 commented Jun 5, 2021 

unsupported: GitHub Container Registry is currently in read only mode. Image pushes are disallowed at this time.

Looks like the container registry is under maintenance currently :(

@Samyak2
Copy link
Member Author

Samyak2 commented Jun 6, 2021

It wooooorks!

https://github.com/orgs/pesos/packages/container/package/grofer

@MadhavJivrajani
Copy link
Member

WOOOHOOOOOOO

MadhavJivrajani
MadhavJivrajani reviewed Jun 6, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
@Samyak2
Copy link
Member Author

Samyak2 commented Jun 6, 2021

I think this is ready now. I made a few changes

  • Docker workflow is disabled for PRs since it will fail unless it's from an internal branch (like this)
  • Changed the README to use ghcr.io/pesos/grofer instead of grofer

@Samyak2 Samyak2 marked this pull request as ready for review June 6, 2021 11:09
@Gituser143
Copy link
Member

@Samyak2 you might want to update the branch :P

@Samyak2
Copy link
Member Author

Samyak2 commented Jun 6, 2021

I'll rebase it on main

@Samyak2 Samyak2 force-pushed the dockerfile branch 2 times, most recently from e0b48a7 to d63865e Compare June 13, 2021 10:42
@Samyak2
Copy link
Member Author

Samyak2 commented Jun 13, 2021

Is there anything left to do here?

@MadhavJivrajani
Copy link
Member

Everything looks good to me, just to be sure: this image build will be triggered only when a new release comes out?

@Samyak2
Copy link
Member Author

Samyak2 commented Jun 13, 2021

No. This is how it will work:

  • All pushes to main (the default branch) are released as the latest image.
  • New tags are released with the specific version number. For example, a 1.3.1 tagged image would be pushed on the next release.

So, pulling ghcr.io/pesos/grofer would give the image from the main branch (since that is tagged as latest). This, if I'm not wrong, is similar to go get which pulls from the default branch.

@MadhavJivrajani
Copy link
Member

MadhavJivrajani commented Jun 13, 2021 •
edited
Loading

Sounds good
Could you add 1-2 lines in the README along the lines of what latest means in our case and if they want to use a particular version, how they can do it?

Samyak2 added a commit that referenced this pull request Jun 14, 2021
@Samyak2
dockerfile (#124) - mention docker tags in README
d23cf41
@Samyak2
Copy link
Member Author

Samyak2 commented Jun 14, 2021

I've added a couple of lines on that.

A few things that should be noted here:

  • The latest image and release images (tags) are not tested xD. They can only be tested once we merge this (and the tag one when we make a new release). act can't test these either since it fails at the login step. We might have to make hotfixes if it breaks. On that note, is it possible to force Actions to run the release (tag) workflow manually so that the 1.3.0 image is built?
  • I'm not entirely sure about the version tag. According to the code (which I stole borrowed from the starter workflows) it should be 1.3.0 without the v prefix. We might have to change this in the README if it's different.

Samyak2 added 12 commits July 5, 2021 15:50
@Samyak2
add Dockerfile with instructions in README
8f840da
@Samyak2
dockerfile - use local dir for build, set all envs
3bf5b5d 
 - instead of go-getting from GitHub, the binary is built from the
   source directory (assumed to be in the same directory as the
   Dockerfile)
 - set all `HOST_` required by gopsutil
 - alternatively, for faster development you can uncomment out the
   `# ADD grofer /go/bin/` to directly add the local `grofer` binary
   instead of building it inside the container everytime.

   If you see a GLIBC error on running the binary in the container,
   build it with `CGO_ENABLED=0 go build`
@Samyak2
dockerfile - update README to use /host instead of /rootfs
3788593 
the `HOST_` env vars are set to `/host` and not `/rootfs`, obviously
that should not have worked lol
@Samyak2
dockerfile - run with --privileged and --pid=host
22aa45b
@Samyak2
dockerfile - switch to alpine from deb-buster
2534a00
@Samyak2
dockerfile - add --network=host flag
d6dbda3 
this fixes wrong network stats issue
@Samyak2
dockerfile - update comments
59ee9bc
@Samyak2
dockerfile - add action to build and pub image
d979331 
 - publishes image to the GitHub docker registry
   (NOT GitHub container registry)
 - temporarily also includes containers for all PRs
    - this is only to test this in the PR
    - will be disabled in a later commit
 - testing with act fails due to the registry
   login step
@Samyak2
dockerfile - ghcr.io based workflow to publish img
3099de3 
 - moved to the GitHub container registry since it supersedes
   the GitHub docker registry
 - act still fails, need to try on actual actions
@Samyak2
dockerfile - add docker socket volume to instrs
d5ad744
@Samyak2
dockerfile - readme points to ghcr.io/pesos/grofer
14c9d7e
@Samyak2
dockerfile - disable docker workflow on PRs
32d7331 
 - this was only enabled for testing
 - it does not currently work for PRs that originate from forks
    - i.e., only internal branches work
 - commented it out in case it is needed later
@Samyak2
Copy link
Member Author

Samyak2 commented Jul 5, 2021

^ Rebased on main

Is there anything else to do here?

MadhavJivrajani
MadhavJivrajani approved these changes Jul 6, 2021
View reviewed changes
Copy link
Member

@MadhavJivrajani MadhavJivrajani left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey, sorry I missed this
I see there are a few small details regarding version to be worked out, but that can be done independently later, with the documentation in the README, this looks good to me in the current state and can be merged

cc @Gituser143

@Gituser143
Copy link
Member

@Samyak2
Changes look good to me! But do workflows trigger on commits? I'm asking because I don't see one for this (or I might not be looking properly, plis help :P).

@Samyak2
Copy link
Member Author

Samyak2 commented Jul 10, 2021

@Samyak2
Changes look good to me! But do workflows trigger on commits? I'm asking because I don't see one for this (or I might not be looking properly, plis help :P).

@Gituser143 yes, but this one is run only on commits to the main branch and not on PRs. It can be enabled on PRs too, but if they aren't from an internal branch of this repo (pesos/grofer), it will just give a permission denied error.
Also, it's run on push rather than commits. So if multiple commits are pushed at a time, it will run only once for the latest commit.

@Gituser143 Gituser143 merged commit 186b32f into main Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gituser143 Gituser143 Gituser143 approved these changes

@MadhavJivrajani MadhavJivrajani MadhavJivrajani approved these changes

@metonymic-smokey metonymic-smokey Awaiting requested review from metonymic-smokey metonymic-smokey is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE REQ] Add Dockerfile for grofer
3 participants
@Samyak2 @MadhavJivrajani @Gituser143