-
Notifications
You must be signed in to change notification settings - Fork 20
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
Frizbee refactoring - support for Dockerfile, docker:// actions, etc. #139
Conversation
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
af21fc3
to
00fa658
Compare
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
04d0852
to
b3c876e
Compare
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
I went through some of the code, not all of it but as you said, a lot of it is moving things around, so I've been trying to orient myself and take notes what is moved where. I think looking at the Minder PR that the API changes are OK and searching GH, Minder is the only API user of frizbee, so if we want to break the API, now is still a good time. But I know we have CLI users (one wrote a post about frizbee). I see that you added aliases for the k8s and dockerhub commands, but what do you think about temporary adding aliases for Also the
and I got:
shouldn't name print something? |
@jhrozek - Thanks for the review!
|
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
@jhrozek - Thanks again! This was indeed a bug - I was accepting the empty string from the flag's default value for overwriting the regex which caused this failure. It should be fixed now 👍 Added more tests cases which bumped the coverage to 69%. |
Draft branches/PRs for the two projects I found that depend on Frizbee - |
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested replacing both "normal" actions and docker actions through and a bunch of things in the CLI. I don't claim I reviewed every single line of the PR, but the API looks good.
I know that @JAORMX was also planning to take a look so don't merge just yet, but ack from my part.
Thanks for all the work, this makes frizbee much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Went through the library usages and tried out the CLI. shipit
The following PR aimed to tidy up the packages, add missing functionality and fix a few of the opened issues.
Details:
docker://
--platform/-p
flag. The code is there but it doesn't work now as there's an opened issue in the underlying library. When debugging the underlying library it turned out that the platform configuration is not really taken into account while fetching the manifest. I can remove the code if needed or mark it as experimental until the issue is resolved.Fixes: #55
Fixes: #67
Fixes: #77
Fixes: #137
Fixes: #138
Closes: #61