support push docker image step by v1v · Pull Request #1550 · elastic/apm-pipeline-library

v1v · 2022-02-15T15:34:09Z

What does this PR do?

Refactor a common step used in Beats, elastic-agent and fleet-server that could benefit us to reduce the complexity in those pipelines with something that's tested accordingly.

You can see how it works in:

refactor pushDockerImages beats#30414

ghost · 2022-02-15T15:48:09Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-02-22T10:38:15.211+0000
Duration: 9 min 19 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1380
Skipped	2
Total	1382

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run infra tests : Run the test-infra test.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

uses elastic/apm-pipeline-library#1550

…eline-library into feature/docker-tag-push-step * 'feature/docker-tag-push-step' of github.com:v1v/apm-pipeline-library: docs: update CHANGELOG.md [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release v1.1.293 fix: update the Kibana build script to the latest changes (#1551)

kuisathaverat · 2022-02-21T10:51:57Z

+* secret: the docker secret
+* registry: the docker registry
+* arch: the supported arch (amd64 or arm64)
+* version: what version
+* snapshot: snapshot support
+* imageName: the docker image name.
+* variants: variants and docker namespace to copy from.
+* targetNamespace: what docker namespace to publish to.


version, arch and variant are complicated, it is not obvious what will be the tag, maybe it worth a few examples to understand how it works

The above example is not enough?

I do not think so, you do not know which Docker image is the source and which will be the destination if you do not check the code.

kuisathaverat · 2022-02-21T10:57:43Z

+  def targetNamespace = args.targetNamespace
+  def registry = args.registry
+
+  def sourceName = "${registry}/${sourceNamespace}/${name}${variant}:${sourceTag}"


Not sure is correct

def registry = 'docker.elastic.co' def sourceNamespace = 'observability-ci' def name = 'metricbeat' variant = 'cloud' sourceTag = '8.0.0-SNAPSHOT' docker.elastic.co/observability-ci/metricbeatcloud:8.0.0-SNPASHOT

I don't understand your question, sourceNamespace is never observability-ci but the one the consumer decides. That's exactly the same implementation as used to be -> https://github.com/elastic/beats/pull/30414/files#diff-5edb5ab93fc95960129eecb2ee3d8763a9c5e02b56e5b80126e0c5a52296ee8fL352

aha! variant should contain -cloud rather than cloud

the values are examples to point you to the separator (-) issue

I don't think that's an issue:

"${registry}/${sourceNamespace}/${name}${variant}:${sourceTag}" then variable names should be defined with the format needed, otherwise what will you use?

kuisathaverat · 2022-02-21T10:58:16Z

+  def registry = args.registry
+
+  def sourceName = "${registry}/${sourceNamespace}/${name}${variant}:${sourceTag}"
+  def targetName = "${registry}/${targetNamespace}/${name}${variant}:${targetTag}"


not sure is correct

def registry = 'docker.elastic.co' def sourceNamespace = 'observability-ci' def name = 'metricbeat' variant = 'cloud' sourceTag = '8.0.0-SNAPSHOT' docker.elastic.co/observability-ci/metricbeatcloud:8.0.0-SNPASHOT

ditto https://github.com/elastic/beats/pull/30414/files#diff-5edb5ab93fc95960129eecb2ee3d8763a9c5e02b56e5b80126e0c5a52296ee8fL315-L321

I'd prefer hiding the - prefix, and let it as an implementation detail:

variants: [ '' : 'beats', 'ubi8' : 'beats', 'cloud' : 'beats-ci', 'complete' : 'beats', ],

and then evaluate it internally when building the image name: if the variant key is not empty, prepend the -. Otherwise the consumer could be adding whatever they like as variant separator, like ~cloud or even worst :cloud. I know this is weird, but I'd see it better if we are controlling the inputs. We could even sanitise them, removing :,\/; etc...

Wdyt?

I understand the concern, but I'd rather prefer the freedom in case there is a corner case to support something else. No strong opinion though

Not a blocker on my side either. We have it already working in the old system and this PR is simply moving it to another shareable place 😄 I'm totally OK with keeping it as is.

kuisathaverat · 2022-02-21T11:01:14Z

+  if (snapshot) {
+    // remove third number in version
+    aliasVersion = version.substring(0, version.lastIndexOf(".")) + "-SNAPSHOT"
+    sourceTag += "-SNAPSHOT"
+  }


I think that this logic should not be here, it is related to process aliases, but not related to pushes Docker images, also, it is not needed if you pass the correct tag.

def version = '8.0.0' aliasVersion -> 8.0-SNAPSHOT

Bear in mind, this implementation did nothing but copy the existing one in the packaging.groovy file, so I didn't want to change the contract

kuisathaverat · 2022-02-21T11:02:04Z

+  def registry = args.containsKey('registry') ? args.registry : error('pushDockerImages: registry parameter is required')
+  def secret = args.containsKey('secret') ? args.secret : error('pushDockerImages: secret parameter is required')
+  def targetNamespace = args.containsKey('targetNamespace') ? args.targetNamespace : error('pushDockerImages: targetNamespace parameter is required')
+  def version = args.containsKey('version') ? args.version : error('pushDockerImages: version parameter is required')


It is stack-oriented variable, it limits the use of the step

kuisathaverat · 2022-02-21T11:06:10Z

+  pushDockerImages(
+    secret: "my-secret",
+    registry: "my-registry",
+    arch: 'amd64',
+    version: '8.2.0',
+    snapshot: true,
+    imageName : 'filebeat',
+    variants: [
+      '' : 'beats',
+      'ubi8' : 'beats',
+      'cloud' : 'beats-ci',
+      'complete' : 'beats',
+    ],
+    targetNamespace: 'observability-ci'
+  )


I think version and arch are redundant, the variants cover the need to push a matrix of Docker images. If we need to specialize the push we can do it in steps designed to implement that layer.

It is more straightforward to have in variants the source tag, the destination tag, and the namespace. even though you have to repeat some configuration values the code will be much simpler and easy to configure.

I understand that the configuration is the one we need to agree, and what do you propose?

v1v · 2022-02-21T11:37:17Z

I do not think so, you do not know which Docker image is the source and which will be the destination if you do not check the code.

See e187d64

mdelapenya · 2022-02-21T11:46:13Z

+    variants: [
+      '' : 'beats',
+      '-ubi8' : 'beats',
+      '-cloud' : 'beats-ci',
+      '-complete' : 'beats',
+    ],


What is the contract to define variant keys and values? I know it's something the consumer should know what to put here, but for having an overall reference, as I'm not able to infer any pattern with the current example

The existing contract was already defined in the packaging.groovy as code:

https://github.com/elastic/beats/blob/452d447372aac5449cf66e05c6e0eaaaf468dd66/.ci/packaging.groovy#L316-L326

This particular variants map provides:

new variant name to be created and what targetNamespace.

Maybe the name could be changed to variantsNamespaceMap if it helps.

I still think this particular implementation could be much simpler if it's done by the build system itself. then from the CI point of view we only need to:

dockerLogin(...) sh 'mage publishDockerImages'

For each beats.

v1v · 2022-02-21T16:16:02Z

+*/
+def call(Map args = [:]) {
+  if(!isUnix()){
+    error('publishToCDN: windows is not supported yet.')


Suggested change

error('publishToCDN: windows is not supported yet.')

error('pushDockerImages: windows is not supported yet.')

v1v · 2022-02-21T16:24:17Z

It is more straightforward to have in variants the source tag, the destination tag, and the namespace. even though you have to repeat some configuration values the code will be much simpler and easy to configure.

Do you mean something like the below snippet?

  pushDockerImages(
    secret: "my-secret",
    version: '8.2.0',
    snapshot: true,
    variantsMatrix: [
      [ source: "my-registry/beats/filebeat", arch: 'amd64', target: "my-registry/observability-ci/filebeat"]
      [ source: "my-registry/beats/filebeat-ubi8", arch: 'amd64', target: "my-registry/observability-ci/filebeat-ubi8"],
      [ source: "my-registry/beats-ci/filebeat-cloud", arch: 'amd64', target: "my-registry/observability-ci/filebeat-cloud"],
      [ source: "my-registry/beats-ci/filebeat-complete", arch: 'amd64', target: "my-registry/observability-ci/filebeat-complete"]
    ]
  )

…push-step * upstream/main: docs: update CHANGELOG.md [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release v1.1.297 Enable uploadPackagesToGoogleBucket step for Beats/ElasticAgent (#1549) feat: improve build Kibana Docker images (#1559)

kuisathaverat · 2022-02-22T16:29:18Z

 ```

-* refspec: A branch (i.e. main), or a pull request identified by the "pr/" prefix and the pull request ID.
+* refspec: A branch (i.e. main), a commit SHA, a tag, or a pull request identified by the "pr/" prefix and the pull request ID.


Tags are not supported, for release tags you can use the official image and for other tags you can use the commit SHA

Suggested change

* refspec: A branch (i.e. main), a commit SHA, a tag, or a pull request identified by the "pr/" prefix and the pull request ID.

* refspec: A branch (i.e. main), a commit SHA, or a pull request identified by the "pr/" prefix and the pull request ID.

I will fix it in other PR is not related with this one

This is unrelated to this PR, since it was generated, let's fix the comment in a follow up

support push docker image step

0daf3b0

mergify bot assigned v1v Feb 15, 2022

v1v mentioned this pull request Feb 15, 2022

ci: publish the packages and docker images elastic/elastic-agent#26

Merged

1 task

pushDockerImages

3c5e90e

v1v added a commit to v1v/beats that referenced this pull request Feb 15, 2022

refactor pushDockerImages

e3e28d2

uses elastic/apm-pipeline-library#1550

v1v mentioned this pull request Feb 15, 2022

refactor pushDockerImages elastic/beats#30414

Merged

v1v and others added 2 commits February 16, 2022 08:39

add echo for debugging purposes

a1db62f

Merge branch 'main' into feature/docker-tag-push-step

1f8dd5e

v1v marked this pull request as ready for review February 16, 2022 08:55

v1v requested review from a team as code owners February 16, 2022 08:55

botelastic bot added the groovy label Feb 16, 2022

v1v added enhancement New feature or request and removed groovy labels Feb 16, 2022

kuisathaverat approved these changes Feb 16, 2022

View reviewed changes

botelastic bot added the groovy label Feb 16, 2022

v1v added 2 commits February 16, 2022 17:55

change signature with a simpler data structure

83f0ee1

v1v added the ready-to-merge label Feb 21, 2022

Merge branch 'main' into feature/docker-tag-push-step

16f96e2

kuisathaverat reviewed Feb 21, 2022

View reviewed changes

v1v commented Feb 21, 2022

View reviewed changes

Comment thread vars/pushDockerImages.txt Outdated

Update vars/pushDockerImages.txt

4f0c192

v1v commented Feb 21, 2022

View reviewed changes

Comment thread vars/pushDockerImages.groovy Outdated

kuisathaverat reviewed Feb 21, 2022

View reviewed changes

Update vars/pushDockerImages.groovy

1c13fc6

mdelapenya reviewed Feb 21, 2022

View reviewed changes

Comment thread vars/pushDockerImages.groovy Outdated

This comment was marked as duplicate.

Sign in to view

v1v added 4 commits February 21, 2022 11:23

sleep with the step itself

0b34a68

remove print

55bcf25

simplify conditional

5b9e384

add example

e187d64

mdelapenya reviewed Feb 21, 2022

View reviewed changes

mock waitUntil with a Map

d8e30f3

v1v removed the ready-to-merge label Feb 21, 2022

v1v commented Feb 21, 2022

View reviewed changes

v1v added 4 commits February 22, 2022 09:19

Simplify the signature

341b7cd

fix typo

7b57541

update docs

981b31e

v1v requested review from kuisathaverat and mdelapenya February 22, 2022 11:57

mdelapenya approved these changes Feb 22, 2022

View reviewed changes

kuisathaverat reviewed Feb 22, 2022

View reviewed changes

kuisathaverat approved these changes Feb 22, 2022

View reviewed changes

v1v merged commit 7480613 into elastic:main Feb 22, 2022

v1v deleted the feature/docker-tag-push-step branch February 22, 2022 16:43

v1v mentioned this pull request Mar 14, 2022

ci: refactor pushDockerImages main pipeline elastic/beats#30795

Merged

	error('publishToCDN: windows is not supported yet.')
	error('pushDockerImages: windows is not supported yet.')

	* refspec: A branch (i.e. main), a commit SHA, a tag, or a pull request identified by the "pr/" prefix and the pull request ID.
	* refspec: A branch (i.e. main), a commit SHA, or a pull request identified by the "pr/" prefix and the pull request ID.

Conversation

v1v commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Uh oh!

ghost commented Feb 15, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Uh oh!

kuisathaverat Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as duplicate.

v1v commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v1v commented Feb 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

v1v commented Feb 15, 2022 •

edited

Loading

ghost commented Feb 15, 2022 •

edited by ghost

Loading

kuisathaverat Feb 21, 2022 •

edited

Loading

v1v commented Feb 21, 2022 •

edited

Loading