feat(plugin): Add ability to push multiple docker images #7

DanielHabenicht · 2019-04-26T22:19:43Z

This feature allows specifying the plugin multiple times in order to push multiple images to a
docker repository.

This feature allows specifying the plugin multiple times in order to push multiple images to a docker repository. iteratec#4

DanielHabenicht · 2019-04-26T22:49:54Z

I don't know why but the tests in the
https://github.com/DanielHabenicht/semantic-release-docker/blob/feat/multiple-images/src/shared-logic.spec.ts
file are not executed.

cmurczek-it · 2019-04-30T16:35:20Z

Hi @DanielHabenicht, with this PR you'd basically revert my changes to mock/stub the docker dependency during unit tests. I've added more test as examples for how I'd like to proceed. Please review these and update your PR.

DanielHabenicht · 2019-05-01T10:53:16Z

Ahh, ok. I think I misunderstood. I will include your changes as units tests and add my tests as e2e Tests.

cmurczek-it · 2019-05-01T12:39:14Z

I actually meant for you to refactor your tests to use sinon for mocking/stubbing, so that testruns are no longer depending on docker being available on build/test agents. An added benefit is that testruns take a LOT less time, eg currently down to 85ms per test run opposed to 1 – 3s per testcase.

…

Sent from my Windows 10 device

________________________________ From: DanielHabenicht <[email protected]> Sent: Wednesday, May 1, 2019 12:53:16 PM To: iteratec/semantic-release-docker Cc: Christoph Murczek; Comment Subject: Re: [iteratec/semantic-release-docker] feat(plugin): Add ability to push multiple docker images (#7) Ahh, ok. I think I misunderstood. I will include your changes as units tests and add my tests as e2e Tests. — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#7 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AGDFOJM2DR27PW535IG7CGTPTFZBZANCNFSM4HI2VTQQ>.

- delete unused auth.ts file - remove docker dependent code in spec files

tests for registry url readded

DanielHabenicht · 2019-06-06T09:26:02Z

Finally, I had some time to refactor the code.
I've used the sinon mocks and stubs in the spec files and made it optional to run the tests that are docker dependent.

I take your point on running the tests a lot faster for development, that's why I refactored it.
But I think it is also important to test the whole plugin (including the dockerode part), that's why I included them as e2e tests that can be run separately (this way we can test if the plugin can interact with a real docker API, thus test if the current version of dockerode does work properly in our use case or even test if the publish step works later on)

Have a look and feel free to comment.
I will refactor the other PR after this gets merged.

cmurczek-it

I do get your motivation for bringing in e2e tests, but this way they provide no benefit over good unit tests. The do however cost time, especially when they are part of every build.
If we added e2e tests they should be truly e2e, meaning actually using semantic-realease with this plugin to build a docker image. However, for now at least I don't see the need for that.

I do like the original idea of the PR though. I'm not sure if the way it's implemented is the best strategy. What's your take on something like
{ plugins: [ prepare: [{ path: "@iteratec/semantic-release-docker", images: [{ additionalTags: [], imageName: "", registryUrl: "", repositoryName: "", }] }] }
To me it feels more natural to configure the plugin once, and have it deal with multiple images, instead of loading multiple instances of the same plugin...

src/verifyConditions/index.ts

cmurczek-it · 2019-07-02T12:40:15Z

src/verifyConditions/index.spec.ts

-      expect(dockerStub.checkAuth.firstCall.args[0]).to.deep.equal(expected);
-    });
-
-    it('should default to docker hub if no registry is specified', async function() {


why would you remove this test? I want to make sure we let docker decide which registry to use if none is specified

Because it basically is a duplicate of "should use the registry from the config" the test checks if the string put into the config is returned. The main difference is that one string is '' and the other is 'my_private_registry'

src/verifyConditions/index.e2e.ts

cmurczek-it · 2019-07-02T20:56:42Z

package.json

    "commit": "git-cz",
    "test": "mocha -r chai -r chai-as-promised -r ts-node/register src/**/*.spec.ts",
+    "e2e": "mocha -r chai -r chai-as-promised -r ts-node/register src/**/*.e2e.ts",


please remove this script

cmurczek-it · 2019-07-02T20:57:30Z

azure-pipelines.yml

        inputs:
          verbose: false
      - task: Npm@1
        displayName: Build
        inputs:
          command: custom
          customCommand: run build
+      - task: Npm@1


remove this task

README.md

.gitignore

cmurczek-it · 2019-07-02T21:10:57Z

Dockerfile

@@ -0,0 +1,2 @@
+FROM scratch
+ARG TEST


Don't burden yourself with creating actual images while developing this plugin. There is no benefit over (correctly) mocking/stubbing dockerode.

DanielHabenicht · 2019-07-17T09:03:58Z

I do get your motivation for bringing in e2e tests, but this way they provide no benefit over good unit tests. They do however cost time, especially when they are part of every build.

There are numerous reasons and benefits for the e2e tests:

The e2e tests actually check on the machine if the plugin has done what it said it did.
With the Unit-Tests only the tools 'said' output is tested. For Example the Bug I fixed before was not detected by your unit tests, because it did only check the tool's output, not the actual behaviour of the tool. That's also the reason why I wrote them in the first place.
The e2e tests are independent of the tools you use (ideally).
You might want to replace dockerode in the future because it gets deprecated or update to the next version. This does change the API of the internally used tools and you have to rewrite your unit tests. While rewriting who checks that the tests and changes you do are correct? (The e2e tests, because they only check the outcome)

If you think the build time is too long I can make them run in parallel. But I think that waiting 22 seconds are worth the amount of time needed for others to test, verify, file a bug and resolve the issue coming from not testing it.

If we added e2e tests they should be truly e2e, meaning actually using semantic-release with this plugin to build a docker image. However, for now, at least I don't see the need for that.

I think of these as integration tests, that's also a good idea but I do align with your view.

I do like the original idea of the PR though. I'm not sure if the way it's implemented is the best strategy. What's your take on something like
{ plugins: [ prepare: [{ path: "@iteratec/semantic-release-docker", images: [{ additionalTags: [], imageName: "", registryUrl: "", repositoryName: "", }] }] }
To me it feels more natural to configure the plugin once, and have it deal with multiple images, instead of loading multiple instances of the same plugin...

I think the way it is implemented is the way to go, with the option to specify multiple images you are bringing in another way of using the plugin, while not fixing the way it already can be used.
For reference the @semantic-release/exec command does it the same way. But it might be a good addition in the future to provide both ways.

- also remove unecessary if condition

DanielHabenicht · 2019-09-03T18:27:10Z

@cmurczek any updates?

DanielHabenicht added 2 commits April 27, 2019 00:18

feat(plugin): Add ability to push multiple docker images

96850c9

This feature allows specifying the plugin multiple times in order to push multiple images to a docker repository. iteratec#4

chore(merge): merge

4e4abe8

This was referenced Apr 26, 2019

feat(plugin): Add new Option to NOT push the version tag #8

Open

fix(plugin): miscellaneous #6

Closed

DanielHabenicht added 5 commits June 5, 2019 23:25

chore(merge): merge without new tests

35e138e

test(prepare): add sinon tests to prepare step

0f6db97

test(e2e): add end 2 end tests to the pipeline

9b055d1

test(verify-conditions): add sinon tests to verifyConditions

4bf28d1

- delete unused auth.ts file - remove docker dependent code in spec files

test(verify-conditions): readd deleted tests

e50bb82

tests for registry url readded

cmurczek-it requested changes Jul 2, 2019

View reviewed changes

DanielHabenicht added 10 commits July 17, 2019 11:10

test(e2e): remove duplicated tests

5d286e8

chore(vscode): add debug task

a0756b8

fix(prepare): remove redundant setting of prepared

d29497b

- also remove unecessary if condition

chore(publish): use getCredentials insted of env

c0d9416

fix(verify-conditions): use Promise instead of for

1033f56

chore(plugin): remove overhead catch functions

8edaa92

test(e2e): fix e2e test

69b0458

chore(merge): merge with master

5913501

chore(readme): update readme

9794172

chore(plugin): move dockerode out of the promise

fdee24d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(plugin): Add ability to push multiple docker images #7

feat(plugin): Add ability to push multiple docker images #7

DanielHabenicht commented Apr 26, 2019

DanielHabenicht commented Apr 26, 2019

cmurczek-it commented Apr 30, 2019

DanielHabenicht commented May 1, 2019

cmurczek-it commented May 1, 2019 via email

DanielHabenicht commented Jun 6, 2019

cmurczek-it left a comment

cmurczek-it Jul 2, 2019

DanielHabenicht Jul 17, 2019

cmurczek-it Jul 2, 2019

cmurczek-it Jul 2, 2019

cmurczek-it Jul 2, 2019

DanielHabenicht commented Jul 17, 2019

DanielHabenicht commented Sep 3, 2019

feat(plugin): Add ability to push multiple docker images #7

Are you sure you want to change the base?

feat(plugin): Add ability to push multiple docker images #7

Conversation

DanielHabenicht commented Apr 26, 2019

DanielHabenicht commented Apr 26, 2019

cmurczek-it commented Apr 30, 2019

DanielHabenicht commented May 1, 2019

cmurczek-it commented May 1, 2019 via email

DanielHabenicht commented Jun 6, 2019

cmurczek-it left a comment

Choose a reason for hiding this comment

cmurczek-it Jul 2, 2019

Choose a reason for hiding this comment

DanielHabenicht Jul 17, 2019

Choose a reason for hiding this comment

cmurczek-it Jul 2, 2019

Choose a reason for hiding this comment

cmurczek-it Jul 2, 2019

Choose a reason for hiding this comment

cmurczek-it Jul 2, 2019

Choose a reason for hiding this comment

DanielHabenicht commented Jul 17, 2019

DanielHabenicht commented Sep 3, 2019