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

chore: refactor findImages to remove print statements #3236

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

a1994sc
Copy link
Contributor

@a1994sc a1994sc commented Nov 13, 2024

Description

Refactor of findImages remove print states from inside the function. It now passes the image map to the devFindImagesCmd which prints a similar table as before.

A regression is that we lost the easy ability add the comment blocks for the maybe images and cosign artifacts.

Related Issue

Relates to #2865

Checklist before merging

@a1994sc a1994sc requested review from a team as code owners November 13, 2024 00:20
Copy link

netlify bot commented Nov 13, 2024

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit 9060366
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/673cf14d60c00100088febb3

if len(images) > 0 {
fmt.Println(IMAGES)
for i := 0; i < len(images); i++ {
fmt.Printf(IMAGE, images[i])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure if printing them out with the fmt.Print functions was the correct way to handle this, I did not see a good log level to put it in the logger.

Please let me know if you want me to update this

@phillebaba
Copy link
Member

We are currently in the process of refactoring the pacakger function so find images will be completely re-written. During this refactoring printing will be deferred to the CLI.

@a1994sc
Copy link
Contributor Author

a1994sc commented Nov 21, 2024

Ah ok, thank you @phillebaba for the clarification. I'm wanting to put some effort into addressing #2865 for automatically updating the zarf.yaml file. Should I assume that findImages will still return an array with the images that zarf discovered still?

@a1994sc
Copy link
Contributor Author

a1994sc commented Nov 21, 2024

I can also close this PR if that is preferred for you

@phillebaba
Copy link
Member

phillebaba commented Nov 21, 2024

I am not looking into changing the existing behavior for find images. So the refactor will still in the end print the found images. the difference being is that the exported library will return a slice instead of printing the results. I would suggest bringing up the issue to change the behavior of find images at the next community meeting of possible.

It will be a lot easier to implement this suggested change after the refactor is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants