Docker files chore #3880

alvicsam · 2021-09-17T14:14:59Z

Renamed docker files with unified naming convention, adjusted .gitlab-ci.yml and documentation.
Moved docker folder to script/docker/polkadot, adjusted scripts and documentation.

Closes https://github.com/paritytech/ci_cd/issues/197

TriplEight

Please add the metadata to https://github.com/paritytech/polkadot/blob/1c0c709a92de263dcf7edadc7c96f2eb847b3c38/scripts/docker/polkadot/manual.Dockerfile and mention a scripts/docker/polkadot/README.md there.
Metadata should be quite similar to the same in ci. and release. Dockerfiles, just mention the type of the image in the description and have different links.
please decide something stable with the naming convention (this is a first time we apply it, and it's not yet really written). So, where to put ci/manual/release/debug before the name or not (collator_ci vs ci_collator), what to use in which case - underscore or hyphen (ci_debug vs staking**_**miner**-**ci). And it would be a good help to write this naming so it would go to the fresh policy document.

chevdor

I don't think the naming of image is appropriate.
For instance an image called ci-debug could belong to any project and do anything...
I commented about the naming in the issue that initiated this PR.

TriplEight · 2021-09-20T14:08:06Z

also commented to the initial issue

chevdor

minor issue with one image name

scripts/docker/staking-miner/staking_miner_injected.Dockerfile

.github/workflows/publish-docker-release.yml

chevdor · 2021-09-22T08:06:53Z

Sorry for the nits but I think this PR is the place set those.

Multiple files vs ARG

Do we really want to have polkadot_injected_release.Dockerfile and polkadot_injected_debug.Dockerfile or could we manage with a single file and an ARG to describe this debug/release ? If we really have 2 files for polkadot, we probably also will need 2 for the collatator, staking-miner, etc... and that will explode the number of files.

Naming

I think we still have a mix in the names. For the staking-miner for instance, it should be everywhere (file names, doc, etc...) staking-miner_foobar and not staking_miner_foobar.

TriplEight · 2021-09-22T14:10:20Z

@chevdor this all sounds good, thank you for your help.
I'd like to add that i.e. the collator_injected.Dockerfile and polkadot_injected_debug.Dockerfile are identical and the difference is only the binary name, we could definitely put it into ARG.
The question here is where to draw the line. We could

put a binary name into ARG and remove every second Dockerfile (i.e. polkadot/collator/staking_miner ones)
put a conditional script into ARG and tell it to run an "injection" logic or "a docker builder pattern". This would leave us with just a couple of Dockerfiles and a script, so no need to maintain pairs of similar files. From this point it becomes increasingly complicated and less readable.
go all-in into templating Dockerfiles and even end up removing all of them in favour of some templating language

I suggest we take the first approach and see if it's good enough.

chevdor

I made quite a bunch of suggestion to help with the maintenance of those images in the future.

chevdor · 2021-09-23T12:35:26Z

scripts/docker/polkadot/polkadot_builder.Dockerfile

+	io.parity.image.source="https://github.com/paritytech/polkadot/blob/master/scripts/docker/polkadot/polkadot_builder.Dockerfile" \
+	io.parity.image.documentation="https://github.com/paritytech/polkadot/scripts/docker/polkadot/README.md"
+
+ARG PROFILE=release


Not a blocker but I ended up removing this ARG in other images as I don't think it makes much sense to build anything but a release version.

Yeah, totally, I think it was there more for explicity.

I initially used that when I made the first images thinking debug image could be useful but that never happened to be really the case.

scripts/docker/polkadot/polkadot_builder.Dockerfile

scripts/docker/polkadot_injected_release.Dockerfile

scripts/docker/staking-miner/staking-miner_builder.Dockerfile

scripts/docker/staking-miner/staking-miner_injected.Dockerfile

chevdor

LGTM, thanks for the big work !

TriplEight · 2021-09-24T12:13:22Z

scripts/docker/polkadot/polkadot_builder.Dockerfile

+LABEL io.parity.image.authors="devops-team@parity.io" \
+	io.parity.image.vendor="Parity Technologies" \
+	io.parity.image.title="builder" \
+	io.parity.image.description="This is the build stage for Polkadot. Here we create the binary." \


The idea behind it was a policy-as-a-code in future that would check that every Dockerfile has metadata. But good point on readability, makes sense to make an exclusion for the files with the specific *builder* in the name for such policy.

TriplEight · 2021-09-24T12:14:37Z

scripts/docker/polkadot/polkadot_builder.Dockerfile

+LABEL io.parity.image.authors="devops-team@parity.io" \
+	io.parity.image.vendor="Parity Technologies" \
+	io.parity.image.title="builder" \
+	io.parity.image.description="This is the build stage for Polkadot. Here we create the binary." \


It's OK to have simple explanation comments instead of the full metadata in such files.

TriplEight · 2021-09-24T13:02:54Z

now you should wait until #3807 gets merged

chevdor · 2021-09-24T14:49:36Z

As a side note (I can open another issue if you prefer), it would be super convenient to have an injected docker image where the binary is built with all features. That would help with try-runtime for instance.

TriplEight · 2021-09-24T14:55:58Z

@chevdor in another PR please

chevdor · 2021-09-24T15:04:07Z

New issue about a new image with all-features: paritytech/polkadot-sdk#942

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

.gitlab-ci.yml

* master: feat: measured oneshots (#3902) remove `AllSubsystems` and `AllSubsystemsGen` types (#3874) Companion for Substrate#9867 (#3938) Substrate Companion for #9552 (#3834) CI: run disputes tests (#3962) Bump parity-scale-codec from 2.3.0 to 2.3.1 (#3959) approval-voting: populate session cache in advance (#3954) Bump libc from 0.2.102 to 0.2.103 (#3950) fix master (#3955) Docker files chore (#3880) Bump nix from 0.19.1 to 0.20.0 (#3587) remove connected disconnected state, 3rd attempt (#3898) fix flaky chain-selection tests (#3948) Add benchmarking for parachain runtime initializer pallet (#3913) minor chore changes (#3944) disputes: reject single-sided disputes (#3903)

* master: (52 commits) Companion for substrate PR#9890 (#3961) Bump version, tx_version and spec_version in prep for v0.9.11 (#3970) Fix master compilation (#3977) Make most XCM APIs accept an Into<MultiLocation> where MultiLocation is accepted (#3627) fix disputes tests (#3974) Drop availability only for candidates that lose disputes (#3973) revert +1 change to be on the safer side (#3972) paras_inherent: reject only candidates with concluded disputes (#3969) feat: measured oneshots (#3902) remove `AllSubsystems` and `AllSubsystemsGen` types (#3874) Companion for Substrate#9867 (#3938) Substrate Companion for #9552 (#3834) CI: run disputes tests (#3962) Bump parity-scale-codec from 2.3.0 to 2.3.1 (#3959) approval-voting: populate session cache in advance (#3954) Bump libc from 0.2.102 to 0.2.103 (#3950) fix master (#3955) Docker files chore (#3880) Bump nix from 0.19.1 to 0.20.0 (#3587) remove connected disconnected state, 3rd attempt (#3898) ...

alvicsam requested review from chevdor, bkchr, TriplEight and a team September 17, 2021 14:14

alvicsam added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Sep 17, 2021

TriplEight suggested changes Sep 20, 2021

View reviewed changes

chevdor suggested changes Sep 20, 2021

View reviewed changes

alvicsam requested review from TriplEight and chevdor September 21, 2021 13:57

chevdor suggested changes Sep 21, 2021

View reviewed changes

scripts/docker/staking-miner/staking_miner_injected.Dockerfile Outdated Show resolved Hide resolved

TriplEight suggested changes Sep 21, 2021

View reviewed changes

.github/workflows/publish-docker-release.yml Outdated Show resolved Hide resolved

chevdor suggested changes Sep 23, 2021

View reviewed changes

chevdor approved these changes Sep 23, 2021

View reviewed changes

TriplEight suggested changes Sep 24, 2021

View reviewed changes

alvicsam requested a review from TriplEight September 24, 2021 12:20

TriplEight approved these changes Sep 24, 2021

View reviewed changes

alvicsam force-pushed the as-docker-chore branch from 4c3d779 to 2e74764 Compare September 24, 2021 15:04

alvicsam added 4 commits September 24, 2021 17:13

Renamed staking-miner Dockerfiles

4e55208

change readme stacking-miner

977a241

mv docker/ to scripts/docker/polkadot

546e6e9

renamed dockerfiles in scripts/docker

ccc229d

alvicsam and others added 17 commits September 24, 2021 17:13

renamed Dockerfiles

b88a80e

Update scripts/docker/staking-miner/staking_miner_injected.Dockerfile

23c5c9d

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

renamed release.Dockerfile

eb9a162

renamed staking-miner dockerfiles

5e759af

add label polkadot_builder.Dockerfile

91afb7e

Update scripts/docker/polkadot/polkadot_builder.Dockerfile

95b2f9e

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

Update scripts/docker/polkadot/polkadot_builder.Dockerfile

af8f2b2

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

Update scripts/docker/polkadot/polkadot_builder.Dockerfile

77843b4

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

Update scripts/docker/polkadot/polkadot_builder.Dockerfile

78546fe

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

Update scripts/docker/polkadot_injected_release.Dockerfile

75c4041

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

Update scripts/docker/collator_injected.Dockerfile

c92cf8e

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

Update scripts/docker/staking-miner/staking-miner_injected.Dockerfile

bde4005

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

Update scripts/docker/staking-miner/staking-miner_builder.Dockerfile

d0f3b09

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

Update scripts/docker/staking-miner/staking-miner_builder.Dockerfile

f74a965

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

Update scripts/docker/staking-miner/staking-miner_injected.Dockerfile

a38854f

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>

move gpg server as variable to GHA config

fadc1fa

simplified polkadot_builder.Dockerfile

f5e84fa

alvicsam force-pushed the as-docker-chore branch from 2e74764 to f5e84fa Compare September 24, 2021 15:14

gitlab-ci modify cp command in test-build-linux-stable

df1d25b

alvicsam requested a review from TriplEight September 27, 2021 08:50

chevdor reviewed Sep 27, 2021

View reviewed changes

.gitlab-ci.yml Outdated Show resolved Hide resolved

alvicsam added 4 commits September 27, 2021 13:56

fix tests

8864054

renamed scripts/docker to scripts/dockerfiles

89b08e1

fix spellcheck

4407296

fix spellcheck

eda4c65

TriplEight merged commit f6826bd into master Sep 28, 2021

TriplEight deleted the as-docker-chore branch September 28, 2021 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docker files chore #3880

Docker files chore #3880

alvicsam commented Sep 17, 2021

TriplEight left a comment •

edited

Loading

chevdor left a comment •

edited

Loading

TriplEight commented Sep 20, 2021

chevdor left a comment

chevdor commented Sep 22, 2021

TriplEight commented Sep 22, 2021

chevdor left a comment

chevdor Sep 23, 2021

TriplEight Sep 24, 2021

chevdor Sep 24, 2021

chevdor left a comment

TriplEight Sep 24, 2021

TriplEight Sep 24, 2021

TriplEight commented Sep 24, 2021

chevdor commented Sep 24, 2021

TriplEight commented Sep 24, 2021

chevdor commented Sep 24, 2021

Docker files chore #3880

Docker files chore #3880

Conversation

alvicsam commented Sep 17, 2021

TriplEight left a comment • edited Loading

Choose a reason for hiding this comment

chevdor left a comment • edited Loading

Choose a reason for hiding this comment

TriplEight commented Sep 20, 2021

chevdor left a comment

Choose a reason for hiding this comment

chevdor commented Sep 22, 2021

Multiple files vs ARG

Naming

TriplEight commented Sep 22, 2021

chevdor left a comment

Choose a reason for hiding this comment

chevdor Sep 23, 2021

Choose a reason for hiding this comment

TriplEight Sep 24, 2021

Choose a reason for hiding this comment

chevdor Sep 24, 2021

Choose a reason for hiding this comment

chevdor left a comment

Choose a reason for hiding this comment

TriplEight Sep 24, 2021

Choose a reason for hiding this comment

TriplEight Sep 24, 2021

Choose a reason for hiding this comment

TriplEight commented Sep 24, 2021

chevdor commented Sep 24, 2021

TriplEight commented Sep 24, 2021

chevdor commented Sep 24, 2021

TriplEight left a comment •

edited

Loading

chevdor left a comment •

edited

Loading