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.

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

[release-1.4] Don't expect the config blob to be listed in (skopeo inspect) #1575

Merged
merged 3 commits into from
Feb 28, 2022

Conversation

mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Feb 17, 2022

This a backport of #1572 to fix CI on this branch.


# Each SHA-named layer file (but not the config) must be listed in the output of 'inspect'.
# As of Skopeo 1.6, (skopeo inspect)'s output lists layer digests,
# but not the digest of the config blob ($config_digest), if any.
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking: This is the 1.4 branch, so the comment is slightly confusing. Is it saying "Prior to 1.6, the output also included the digest of the config blob"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is meant to say “as of all existing versions to date, up to and including 1.6”.

Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

LGTM

config_digest=$(jq -r '.config.digest' <<<"$inspect_local_raw")

# Each SHA-named layer file (but not the config) must be listed in the output of 'inspect'.
# As of Skopeo 1.6, (skopeo inspect)'s output lists layer digests,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# As of Skopeo 1.6, (skopeo inspect)'s output lists layer digests,
# As of Skopeo 1.6, (skopeo inspect's) output lists layer digests,

Copy link
Member

Choose a reason for hiding this comment

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

Although I think I'd just dro the parens all together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parens are a somewhat-weird habit to quote shell commands in a way where copy-pasting both with and without the parens is a valid shell command (unlike using ", or ', let alone typographically-correct quotes); so moving the 's inside doesn’t work.

The parens are an habit from a specific niche period of the Fedora IRC universe, obscure enough to be confusing, I’ll drop them.

@TomSweeneyRedHat
Copy link
Member

Doc nits for consideration, ditto 1.13 variant. LGTM otherwise and I'd be fine with the docs as is.

@mtrmac
Copy link
Contributor Author

mtrmac commented Feb 21, 2022

These PRs are supposed to be backports — I’ll prepare fixes on the main branch, and add them to these PRs.

@mtrmac
Copy link
Contributor Author

mtrmac commented Feb 22, 2022

These PRs are supposed to be backports — I’ll prepare fixes on the main branch,

#1576 .

@mtrmac
Copy link
Contributor Author

mtrmac commented Feb 23, 2022

Updated by adding #1576.

@rhatdan
Copy link
Member

rhatdan commented Feb 28, 2022

LGTM

@rhatdan rhatdan merged commit c5b6140 into containers:release-1.4 Feb 28, 2022
@mtrmac mtrmac deleted the inspect-expect-config-1.4 branch February 28, 2022 15:31
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants