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

remote: fix manifest add --annotation #16004

Merged
merged 1 commit into from
Nov 10, 2022
Merged

remote: fix manifest add --annotation #16004

merged 1 commit into from
Nov 10, 2022

Conversation

sstosh
Copy link
Contributor

@sstosh sstosh commented Sep 30, 2022

  • manifest add --annotation option adds annotations field on remote environment.
  • manifest inspect prints annotations field on remote environment.

Fixes: #15952

Signed-off-by: Toshiki Sonoda [email protected]

This PR is related to #15988.

Does this PR introduce a user-facing change?

Now manifest add --annotation option adds annotations field and
manifest inspect prints annotations field on remote environment.

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Sep 30, 2022
@rhatdan
Copy link
Member

rhatdan commented Sep 30, 2022

@vrothberg @Luap99 PTAL

mheon
mheon reviewed Sep 30, 2022
View reviewed changes
pkg/domain/entities/manifest.go
@@ -36,7 +36,7 @@ type ManifestAddOptions struct {
// ManifestAnnotateOptions provides model for annotating manifest list
type ManifestAnnotateOptions struct {
// Annotation to add to manifest list
Annotation []string `json:"annotation" schema:"annotation"`
Copy link
Member

Choose a reason for hiding this comment

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

@Luap99 This'd be a breaking change, right?

Copy link
Member

Choose a reason for hiding this comment

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

This should be OK. pkg/domain/entities is an internal package.

Copy link
Member

Choose a reason for hiding this comment

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

No this is breaking the rest API, the structs in pkg/domain/entities are sometimes used in the API to unmarshal the query.
see

podman/pkg/domain/entities/manifest.go

Lines 18 to 19 in 226977f

type ManifestAddOptions struct {
ManifestAnnotateOptions
and

podman/pkg/domain/entities/manifest.go

Lines 63 to 65 in 226977f

type ManifestModifyOptions struct {
Operation string `json:"operation" schema:"operation"` // Valid values: update, remove, annotate
ManifestAddOptions

which is then parsed in

podman/pkg/api/handlers/libpod/manifests.go

Lines 435 to 436 in 226977f

body := new(entities.ManifestModifyOptions)
if err := json.NewDecoder(r.Body).Decode(body); err != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review.
I'll think about other methods.

Copy link
Contributor Author

@sstosh sstosh Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I fixed it.
Add an Annotations field while keeping an Annotate Field.
Because of Decode() fails to unmarshal.

@sstosh sstosh force-pushed the remote-manifest-annotate branch 2 times, most recently from aa70e57 to 4e5afd0 Compare October 3, 2022 09:10
@rhatdan
Copy link
Member

rhatdan commented Oct 28, 2022

@vrothberg @Luap99 where are we on this one?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2022
@sstosh sstosh force-pushed the remote-manifest-annotate branch from 4e5afd0 to f2d3378 Compare November 8, 2022 08:12
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2022
@mheon
Copy link
Member

mheon commented Nov 8, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 8, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, sstosh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2022
mheon
mheon reviewed Nov 8, 2022
View reviewed changes
pkg/domain/entities/manifest.go Outdated
@@ -43,6 +43,8 @@ type ManifestAddOptions struct {
type ManifestAnnotateOptions struct {
// Annotation to add to manifest list
Annotation []string `json:"annotation" schema:"annotation"`
// Annotation to add to manifest list by a map
Copy link
Member

Choose a reason for hiding this comment

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

Can you add that annotations is preferred over annotation to the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@sstosh sstosh force-pushed the remote-manifest-annotate branch from f2d3378 to f807b67 Compare November 9, 2022 04:24
@sstosh
remote: fix manifest add --annotation
f807b67 
* `manifest add --annotation option` adds annotations
  field on remote environment.
* `manifest inspect` prints annotations field
  on remote environment.

Fixes: containers#15952

Signed-off-by: Toshiki Sonoda <[email protected]>
@mheon
Copy link
Member

mheon commented Nov 9, 2022

@containers/podman-maintainers PTAL

@rhatdan
Copy link
Member

rhatdan commented Nov 9, 2022

LGTM

vrothberg
vrothberg reviewed Nov 10, 2022
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

Thank you!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2022
@openshift-merge-robot openshift-merge-robot merged commit e86cef1 into containers:main Nov 10, 2022
@sstosh sstosh deleted the remote-manifest-annotate branch November 10, 2022 10:49
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@vrothberg vrothberg vrothberg left review comments

@mheon mheon mheon left review comments

Assignees

@vrothberg vrothberg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remote: manifest add --annotation failed to add annotations field
6 participants
@sstosh @rhatdan @mheon @vrothberg @Luap99 @openshift-merge-robot