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

docker: parametrize risk and OS for image builds (core18 etc) #2673

Merged
merged 2 commits into from Apr 5, 2021

Conversation

abitrolly
Copy link
Contributor

@abitrolly abitrolly commented Aug 20, 2019

This PR allows to change Ubuntu version and risk level of docker image from command line without editing Dockerfiles as advised at https://forum.snapcraft.io/t/creating-docker-images-for-snapcraft/11739

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh tests/unit?

@abitrolly abitrolly changed the title Single dockerfile Parametrize docker builds Aug 22, 2019
ppd added a commit to ppd/solvespace-snapcraft-docker that referenced this pull request Aug 25, 2019
@codecov-io
Copy link

codecov-io commented Aug 27, 2019

Codecov Report

Merging #2673 into master will increase coverage by 1.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2673      +/-   ##
==========================================
+ Coverage    87.9%   89.19%   +1.29%     
==========================================
  Files         229      210      -19     
  Lines       16317    14313    -2004     
  Branches     2486     2166     -320     
==========================================
- Hits        14343    12767    -1576     
+ Misses       1436     1094     -342     
+ Partials      538      452      -86
Impacted Files Coverage Δ
snapcraft/internal/pluginhandler/_patchelf.py 52.77% <0%> (-13.36%) ⬇️
snapcraft/project/_project_info.py 94.23% <0%> (-5.77%) ⬇️
snapcraft/cli/_options.py 81.39% <0%> (-5.34%) ⬇️
snapcraft/internal/lifecycle/_packer.py 73.01% <0%> (-4.77%) ⬇️
...ader/grammar_processing/_part_grammar_processor.py 91.3% <0%> (-4.35%) ⬇️
snapcraft/internal/project_loader/grammar/_to.py 78.78% <0%> (-3.04%) ⬇️
snapcraft/internal/project_loader/grammar/_on.py 79.41% <0%> (-2.95%) ⬇️
snapcraft/cli/_errors.py 85.06% <0%> (-1.65%) ⬇️
...raft/internal/project_loader/_extensions/_utils.py 95.6% <0%> (-1.07%) ⬇️
snapcraft/cli/lifecycle.py 86.76% <0%> (-0.97%) ⬇️
... and 88 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2252dd7...26bd7de. Read the comment docs.

@abitrolly
Copy link
Contributor Author

Rebased.

@sergiusens
Copy link
Collaborator

This will need conflict resolution work applied. Thanks

@abitrolly
Copy link
Contributor Author

@sergiusens ready.

@sergiusens
Copy link
Collaborator

Mind telling me how to configure hub.docker.com to automatically build with a matrix of environment variables?

@abitrolly
Copy link
Contributor Author

@sergiusens is there a problem to configure 4 builds with separate environment variables manually? https://docs.docker.com/docker-hub/builds/#environment-variables-for-builds

I don't use Docker Hub autobuilds. I push images from Travis after their tests are finished. Otherwise you will also need to configure testing on Docker Hub.

The CI configuration for snapcraft Docker builds is not using code in repository, but uses files from snap feteched from snapstore API. Building this on every commit is not efficient, and could be improved by using repository tags or by additional checks that could be impossible for Docker Hub, which doesn't know anything about snap versions.

@abitrolly abitrolly changed the title Parametrize docker builds docker: parametrize risk and OS for image builds (core18 etc) Jan 2, 2020
@abitrolly
Copy link
Contributor Author

Rebased. Should work for core20 too.

@knocte
Copy link

knocte commented May 24, 2020

Shouldn't the X-Ubuntu-Series header be parameterised too? It's hardcoded to 16 now

@sergiusens
Copy link
Collaborator

We are going to close this PR due to lack of activity.
We don't have anything currently outside of dockerhub that sets up these environments.
If you rather create a docker file independent of these to carry in our repo that people could base out from then that would certainly be welcomed.

@sergiusens sergiusens closed this Sep 28, 2020
@knocte
Copy link

knocte commented Sep 29, 2020

Lack of activity? WTF? There's lack of activity because there's lack of PR review, which is not to blame to the PR creator.

@cjp256
Copy link
Contributor

cjp256 commented Sep 29, 2020

Sorry about that, you're right. I'm going to reopen this and see what we can do. I like the consolidation of the Dockerfiles, but do not fully understand the effects on our CI & images yet.

I started experimenting with the Docker images a couple months ago incorporating the approach taken here as the starting point, but have not yet found the time to get back to it. https://github.com/cjp256/snapcraft-docker-image/blob/master/Dockerfile.

I'll do my best to put some cycles on it next week.

@cjp256 cjp256 reopened this Sep 29, 2020
@abitrolly
Copy link
Contributor Author

I've rebased this 5 times already over this year. I feel like I just need to apply to be a product owner for Docker users in this project. )

@sergiusens
Copy link
Collaborator

Thank you for your patience, I will merge as soon as conflicts are resolved (last time 🤞)

@knocte
Copy link

knocte commented Mar 31, 2021

@sergiusens can you answer #2673 (comment)?

@sergiusens
Copy link
Collaborator

@sergiusens can you answer #2673 (comment)?

It should not, the name is unfortunate, but it relates to API version around snaps

Signed-off-by: Anatoli Babenia <anatoli@rainforce.org>
To avoid changing Dockerfile as described in
https://forum.snapcraft.io/t/creating-docker-images-for-snapcraft/11739

Signed-off-by: Anatoli Babenia <anatoli@rainforce.org>
@abitrolly
Copy link
Contributor Author

@sergiusens conflict are resolved. Last time! :D

@abitrolly
Copy link
Contributor Author

CLA check service is down. I signed that before
Uploading image.png…

@sergiusens
Copy link
Collaborator

@MarcusTomlinson fyi, seems CLA check thing is down

@MarcusTomlinson
Copy link
Contributor

@MarcusTomlinson fyi, seems CLA check thing is down

Yeah, was an issue in LP (https://bugs.launchpad.net/launchpad/+bug/1921727), should be fixed now.

@sergiusens
Copy link
Collaborator

@abitrolly did you use your same email to author the commit?
@MarcusTomlinson any idea how this could fail?

@abitrolly
Copy link
Contributor Author

@sergiusens everything is the same. You can compare with any https://github.com/snapcore/snapcraft/pulls?q=is%3Apr+author%3Aabitrolly+is%3Aclosed

@MarcusTomlinson
Copy link
Contributor

MarcusTomlinson commented Apr 2, 2021

@abitrolly did you use your same email to author the commit?
@MarcusTomlinson any idea how this could fail?

The GitHub user: https://github.com/abitrolly is not a member of: https://github.com/CanonicalContributorAgreement
The Launchpad user: https://launchpad.net/~abitrolly is not a member of: https://launchpad.net/~contributor-agreement-canonical

Please signed the Canonical Contributor Agreement here: https://ubuntu.com/legal/contributors/agreement

(Note that even once you've signed the agreement, it can take a few hours for you to be added)

@abitrolly
Copy link
Contributor Author

@MarcusTomlinson but it was. So what happened?

@sergiusens
Copy link
Collaborator

We are doing better CLA checking now. One minor commit made it into master and our previous check completed skipped the check so it seems you had never signed the CLA. Since this change is bigger than a minor change, we would require you to sign it before merging this. If proceeding, I would be a valid contact which is required in the form.

@abitrolly
Copy link
Contributor Author

Signed again.

@sergiusens sergiusens merged commit b982555 into canonical:master Apr 5, 2021
@sergiusens
Copy link
Collaborator

I checked history, and it seems it passed before as there were commits on master which is not longer looked at in the new checks

@abitrolly abitrolly deleted the single-dockerfile branch April 5, 2021 19:20
@knocte
Copy link

knocte commented Apr 6, 2021

Should the docs be changed now that this PR has been merged?

@abitrolly
Copy link
Contributor Author

@knocte which docs?

@abitrolly
Copy link
Contributor Author

I see that both https://snapcraft.io/docs/snapcraft-docker-images and https://github.com/snapcore/snapcraft/tree/master/docker need to mention how to build for specific Ubuntu core. I can only send PR for the contents of README.md.

abitrolly added a commit to abitrolly/snapcraft that referenced this pull request Apr 10, 2021
Leave only relevant details and mention how to specify Ubuntu version

Addresses canonical#2673 (comment)
@abitrolly abitrolly mentioned this pull request Apr 10, 2021
4 tasks
@abitrolly
Copy link
Contributor Author

I sent the PR to update README.md, and somebody else needs to take care about the forum/doc contents.

abitrolly added a commit to abitrolly/snapcraft that referenced this pull request Apr 15, 2021
Leave only relevant details and mention how to specify Ubuntu version

Addresses canonical#2673 (comment)
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.

None yet

6 participants