Skip to content
This repository was archived by the owner on Oct 23, 2025. It is now read-only.

Conversation

@rafaelcaricio
Copy link
Contributor

@rafaelcaricio rafaelcaricio commented May 31, 2016

@coveralls
Copy link

coveralls commented May 31, 2016

Coverage Status

Coverage decreased (-0.2%) to 85.997% when pulling 7c31826 on rafaelcaricio:show-open-cves-on-create into 8a1b1d9 on zalando-stups:master.

if image_tag['severity_fix_available'] not in ['NOT_PROCESSED_YET',
'COULDNT_FIGURE_OUT',
'NO_CVES_FOUND',
'TOO_OLD']:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarnowski Could you review that those stuatuses are the ones we do not want to show the warning? I am not sure about the TOO_OLD and NOT_PROCESSED_YET maybe we want to show the warning for those statuses too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes both should warn the user as well. It doesn't prevent the user from doing it anyway.

@hjacobs
Copy link
Contributor

hjacobs commented Jun 6, 2016

@rafaelcaricio please update zalando-stups/pierone-cli#38



def check_docker_image_exists(docker_image: pierone.api.DockerImage):
if 'pierone' in docker_image.registry:
Copy link
Contributor

Choose a reason for hiding this comment

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

this actually will not trigger for OS registry (registry.opensource.zalan.do) --- this check is meant to check whether we need authentication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... right. I will try another approach then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the approach! Maybe now is even better because we will show a warning when user is trying to deploy an image that is not from Pierone.

@rafaelcaricio
Copy link
Contributor Author

@hjacobs done.

@rafaelcaricio
Copy link
Contributor Author

@hjacobs @sarnowski please review.

@hjacobs
Copy link
Contributor

hjacobs commented Jun 6, 2016

@rafaelcaricio can we have a test please?


if not exists:
raise click.UsageError('Docker image "{}" does not exist'.format(docker_image))
elif 'severity_fix_available' in image_tag:
Copy link
Contributor

Choose a reason for hiding this comment

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

image_tag might not be defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😱 You're right! I am changing 4 projects at same time... I need to focus better.

@hjacobs
Copy link
Contributor

hjacobs commented Jun 6, 2016

And what about OS registry now?

@rafaelcaricio
Copy link
Contributor Author

@hjacobs Now it handles any docker image registry, if it does not find the pierone endpoints it will just assume it is not pierone. 😷

try:
exists = pierone.api.image_exists('pierone', docker_image)
except pierone.api.Unauthorized:
token = get_existing_token('pierone')
Copy link
Contributor

Choose a reason for hiding this comment

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

this will not work for service tokens

Copy link
Contributor Author

@rafaelcaricio rafaelcaricio Jun 7, 2016

Choose a reason for hiding this comment

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

Why? That is the exact code that pierone-cli was calling inside image_exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I make it work?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, might be, so it would just work for human users --- you can use zign.api.get_token(...) which tries best effort and also works with service tokens

@hjacobs
Copy link
Contributor

hjacobs commented Jun 8, 2016

👍

@hjacobs hjacobs merged commit 727466a into zalando-stups:master Jun 8, 2016
@rafaelcaricio rafaelcaricio deleted the show-open-cves-on-create branch June 8, 2016 08:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants