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

Refactor resources "dockerimage" and "dockerdigest" into a new package "dockerimage" #418

Merged
merged 5 commits into from
Jan 5, 2022
Merged

Refactor resources "dockerimage" and "dockerdigest" into a new package "dockerimage" #418

merged 5 commits into from
Jan 5, 2022

Conversation

dduportal
Copy link
Contributor

@dduportal dduportal commented Dec 15, 2021 •
edited

This PR comes from the work in #412 .

It introduces the following changes:

Functional changes:

  • Use the Registry API v2 for all dockerdigest and dockerimage calls:
    • Deprecates the "Bearer Token" specification in favor of username + password (or anonymous), as it's generated automatically as part of the V2 API
    • Fully support Architecture for images (bugfix for DockerHub images, new feature for quay.io images)

Technical changes:

  • Add manifest examples to allow (manual) end to end testing for both providers

  • Introduce a method New(Spec) to allow initializing an object with a state

  • Introduces a utility docker package with sub-packages

    • Defines the default values for image naming
    • Introduces docker/dockerimage sub-package to manage Docker images parsing logic
    • Introduces docker/dockerregistry sub-package to manage registry API logic

  • Introduces packages to allow unit testing with mocks from HTTP clients and Docker utilities:

    • Introduces docker/dockermocks sub-package to provides mocks for DockerRegistry logic
    • Introduces pkg/core/httpclient package to define HTTP client interface (and allows mocking it on each package)

  • Refactor dockerdigest resource:

    • In its own package, using the docker/* utilities
    • Add a New constructor method
    • Add unit tests with >80% code coverage

  • Refactor dockerimage resource:

    • In its own package, using the docker/* utilities
    • Add a New constructor method
    • Add unit tests with >80% code coverage

Test

To test this pull request, you can run the following commands:

go build -o dist/updatecli
./dist/updatecli diff --config ./examples/updatecli.generic/dockerdigest.yaml
./dist/updatecli diff --config ./examples/updatecli.generic/dockerimage.yaml

Additional Information

Tradeoff

Potential improvement

@dduportal dduportal marked this pull request as draft December 15, 2021 17:59
@dduportal dduportal force-pushed the refactor/plugin-dockerdigest branch 3 times, most recently from 6a1bebc to d8fec75 Compare December 22, 2021 07:51
@olblak
Copy link
Member

olblak commented Dec 24, 2021

While you are into this, you may have a look to #432 (comment)

@dduportal dduportal mentioned this pull request Dec 24, 2021
Closed
@dduportal
Copy link
Contributor Author

While you are into this, you may have a look to #432 (comment)

Yep, good thinking! Answered: #432 (comment)

@dduportal dduportal marked this pull request as ready for review January 1, 2022 15:51
@dduportal dduportal added enhancement New feature or request resource-dockerdigest Resource of kind dockerDigest resource-docker Resource of kind Docker Image resource-dockerfile Resource of kind Dockerfile labels Jan 1, 2022
@dduportal
Copy link
Contributor Author

Ping @olblak this one is ready for review \o/

@dduportal
Copy link
Contributor Author

@olblak this PR does not have a specific issue, so I wasn't able to propose a milestone.

WDYT about 0.18.0 ?

@dduportal
chore(dockerdigest,dockerimage) add example manifests for end to end …
7a3e9ea 
…tests

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal
chore(dockerfile) refactor dockerfile plugin into its own package
8a54cd1 
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal
feat(dockerdigest,dockerimage) refactor docker(.*) resources into the…
3bbb72e 
…ir own packages to fully supports image architectures

== Functional changes:

- Use the Registry API v2 for all dockerdigest and dockerimage calls:
  * Deprecates the "Bearer Token" specification in favor of username + password (or anonymous), as it's generated automatically as part of the V2 API
  * Fully support Architecture for images (bugfix for DockerHub images, new feature for quay.io images)

== Technical changes:

- Introduces a utility `docker` package with sub-packages
  * Defines the default values for image naming
  * Introduces `docker/dockerimage` sub-package to manage Docker images parsing logic
  * Introduces `docker/dockerregistry` sub-package to manage registry API logic

- Introduces packages to allow unit testing with mocks from HTTP clients and Docker utilities:
  * Introduces `docker/dockermocks` sub-package to provides mocks for DockerRegistry logic
  * Introduces `pkg/core/httpclient` package to define HTTP client interface (and allows mocking it on each package)

- Refactor dockerdigest resource:
  * In its own package, using the docker/* utilities
  * Add a `New` constructor method
  * Add unit tests with >80% code coverage

- Refactor dockerimage resource:
  * In its own package, using the docker/* utilities
  * Add a `New` constructor method
  * Add unit tests with >80% code coverage

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@olblak
Copy link
Member

olblak commented Jan 4, 2022

@olblak this PR does not have a specific issue, so I wasn't able to propose a milestone.

WDYT about 0.18.0 ?

Yep let exceptionally set the milestone on the pullrequest

@olblak olblak added this to the 0.18.0 milestone Jan 4, 2022
@olblak
Copy link
Member

olblak commented Jan 4, 2022

This pullrequest is a very nice refactoring especially considering the lack of documentation around docker library

olblak
olblak previously approved these changes Jan 4, 2022
View reviewed changes
@dduportal @olblak
Update examples/updatecli.generic/dockerdigest.yaml
f3bc5a8 
Co-authored-by: Olivier Vernin <me@olblak.com>
@dduportal dduportal enabled auto-merge (rebase) January 4, 2022 10:53
@dduportal dduportal requested a review from olblak January 4, 2022 11:19
@dduportal dduportal enabled auto-merge (rebase) January 4, 2022 11:33
@dduportal
Copy link
Contributor Author

@olblak can you approve-it again? It's in "auto merge" but by adding your suggestion cancelled your previous approval

olblak
olblak approved these changes Jan 5, 2022
View reviewed changes
Copy link
Member

@olblak olblak left a comment

Choose a reason for hiding this comment

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

:shipit:

@dduportal dduportal merged commit 5b7cdf8 into updatecli:main Jan 5, 2022
@dduportal dduportal deleted the refactor/plugin-dockerdigest branch January 5, 2022 06:18
This was referenced Jan 28, 2022
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olblak olblak olblak approved these changes

Assignees
No one assigned
Labels
chore cleanup enhancement New feature or request resource-docker Resource of kind Docker Image resource-dockerdigest Resource of kind dockerDigest resource-dockerfile Resource of kind Dockerfile
Projects
None yet
Milestone
0.18.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dduportal @olblak