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

Adding crane ls command #1835

Merged
merged 10 commits into from
Jun 26, 2023
Merged

Adding crane ls command #1835

merged 10 commits into from
Jun 26, 2023

Conversation

dgershman
Copy link
Contributor

Description

This adds support for crane ls. This feature is very useful when attempting to do some automation of updating images. The only other way to do this is to run zarf connect registry and then request the tags endpoint of the registry.

Related Issue

N/A

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@netlify
Copy link

netlify bot commented Jun 21, 2023 •
edited
Loading

✅ Deploy Preview for zarf-docs ready!

Name Link
🔨 Latest commit 4fac284
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/6499da6b1cf8e000089c034f
😎 Deploy Preview https://deploy-preview-1835--zarf-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dgershman dgershman changed the title Feature/crane ls command Adding crane ls command Jun 21, 2023
YrrepNoj
YrrepNoj suggested changes Jun 21, 2023
View reviewed changes
src/cmd/tools/crane.go Outdated Show resolved Hide resolved
src/cmd/tools/crane.go Outdated Show resolved Hide resolved
YrrepNoj
YrrepNoj suggested changes Jun 22, 2023
View reviewed changes
src/cmd/tools/crane.go Outdated Show resolved Hide resolved
src/cmd/tools/crane.go Outdated Show resolved Hide resolved
@dgershman dgershman force-pushed the feature/crane-ls-command branch from ecfdc1b to d2a85c7 Compare June 22, 2023 22:13
Racer159
Racer159 requested changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@Racer159 Racer159 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think the logic overall looks good - added some feedback on internal vs external registry support. Also you'll need to run make docs-and-schema to update the CLI docs. Also as a heads up / note this may hit some small merge conflicts with #1848 as well (edit: the conflicts were really minor so I merged it in)

src/cmd/tools/crane.go Outdated Show resolved Hide resolved
src/cmd/tools/crane.go Outdated Show resolved Hide resolved
src/cmd/tools/crane.go Outdated Show resolved Hide resolved
src/cmd/tools/crane.go Outdated Show resolved Hide resolved
Racer159
Racer159 previously approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@Racer159 Racer159 left a comment

Choose a reason for hiding this comment

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

lgtm

@Racer159 Racer159 self-requested a review June 26, 2023 18:00
@dgershman dgershman force-pushed the feature/crane-ls-command branch from f7c3f0e to 068f846 Compare June 26, 2023 18:00
@dgershman dgershman requested a review from Madeline-UX as a code owner June 26, 2023 18:00
Racer159
Racer159 requested changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@Racer159 Racer159 left a comment

Choose a reason for hiding this comment

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

make docs-and-schema looks like it still needs to be ran

Racer159
Racer159 approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@Racer159 Racer159 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm (ah GitHub race conditions...)

@dgershman
Copy link
Contributor Author

I am seeing an issue with make docs-and-schema where its updating all the docs with an extra line break at the end of each markdown file.

@Racer159
Copy link
Contributor

I am seeing an issue with make docs-and-schema where its updating all the docs with an extra line break at the end of each markdown file.

Hmm that might be because of your OS... What were you devving on?

@dgershman
Copy link
Contributor Author

dgershman commented Jun 26, 2023 •
edited
Loading

macOS Ventura 13.4

@Racer159
Copy link
Contributor

macOS Ventura 13.4

Hmm I mostly run flavors of Linux - might be something with the sed command we run under docs/gen-cli-docs.sh

@Racer159
Copy link
Contributor

Looks like if you remove one newline from the doc you added that would make it pass.

@dgershman
Copy link
Contributor Author

macOS Ventura 13.4

Hmm I mostly run flavors of Linux - might be something with the sed command we run under docs/gen-cli-docs.sh

Yah. Fixed on my end with an alias to gsed. https://superuser.com/questions/307165/newlines-in-sed-on-mac-os-x

Racer159
Racer159 requested changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@Racer159 Racer159 left a comment

Choose a reason for hiding this comment

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

image

Looks like catalog and registry also changed (since ls is a new subcommand of registry).

@Racer159
Copy link
Contributor

image

Looks like catalog and registry also changed (since ls is a new subcommand of registry).

(might still need catalog to be updated but we'll see... GH doesn't do a good job of showing updates when they happen)

Racer159
Racer159 approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@Racer159 Racer159 left a comment

Choose a reason for hiding this comment

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

lgtm! 🎉

@Racer159 Racer159 requested a review from YrrepNoj June 26, 2023 18:45
@Racer159 Racer159 enabled auto-merge (squash) June 26, 2023 18:48
@dgershman
Copy link
Contributor Author

dgershman commented Jun 26, 2023 •
edited
Loading

PS, I plan to work in crane push and crane pull (as a separate PR) so they also don't require establishing a tunnel if they are referencing an internal image. Is this cool?

@Racer159
Copy link
Contributor

Yep! That works following the pattern established here. Will definitely make the crane commands more useful.

@Racer159 Racer159 merged commit 09522c4 into zarf-dev:main Jun 26, 2023
@dgershman dgershman mentioned this pull request Jun 26, 2023
Merged
5 tasks
Racer159 added a commit that referenced this pull request Jun 28, 2023
@dgershman @Racer159
Allow for internal crane pulls and pushes without a tunnel + explicit…
dd54f24 
… auth (#1851)

Allow for internal crane pulls and pushes without a tunnel + explicit
credentials

## Description

This gives the same ability as #1835 to `zarf tools crane push` and
`zarf tools crane pull`. Essentially, you shouldn't have to create a
tunnel and auth to the registry to pull or push images to it.

## Related Issue

#1835

## Type of change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Other (security config, docs update, etc)

## Checklist before merging

- [x] Test, docs, adr added or updated as needed
- [x] [Contributor Guide
Steps](https://github.com/defenseunicorns/zarf/blob/main/CONTRIBUTING.md#developer-workflow)
followed

Co-authored-by: Wayne Starr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YrrepNoj YrrepNoj YrrepNoj approved these changes

@Racer159 Racer159 Racer159 approved these changes

@jeff-mccoy jeff-mccoy Awaiting requested review from jeff-mccoy

@Noxsios Noxsios Awaiting requested review from Noxsios

@Madeline-UX Madeline-UX Awaiting requested review from Madeline-UX

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dgershman @Racer159 @YrrepNoj