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

fix: inspect function support resolving shortname #2551

Merged
merged 1 commit into from
Feb 11, 2023
Merged

Conversation

24sama
Copy link
Contributor

@24sama 24sama commented Feb 10, 2023

Fix #2524

Currently, there is an error when executing the run command with a short name:

image

This PR adds the localhost and docker.io prefixes to the shortname.

image

More info about the shortname, please see: containers/buildah#1034

@cla-assistant
Copy link

cla-assistant bot commented Feb 10, 2023

CLA assistant check
All committers have signed the CLA.

@cla-assistant
Copy link

cla-assistant bot commented Feb 10, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@24sama 24sama force-pushed the main branch 2 times, most recently from 9c6da4b to 763079c Compare February 10, 2023 09:46
@24sama 24sama changed the title inspect function support resolving shortname fix: inspect function support resolving shortname Feb 10, 2023
@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Base: 65.09% // Head: 65.09% // No change to project coverage 👍

Coverage data is based on head (641fc72) compared to base (d68fa93).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2551   +/-   ##
=======================================
  Coverage   65.09%   65.09%           
=======================================
  Files           8        8           
  Lines         573      573           
=======================================
  Hits          373      373           
  Misses        168      168           
  Partials       32       32           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zzjin
Copy link
Collaborator

zzjin commented Feb 10, 2023

Thank you for your first contribution!

return nil, nil, err
return nil, nil, fmt.Errorf("error resolve shortname %s: %w", imgName, storage.ErrImageUnknown)
}
// also try out the "docker.io/" prefixed image
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the ResolveLocally functional, but seems we do not need to auto parse imgName for docker.io with two reasons below:

  1. If the image belongs to quay.io or ghcr.io
  2. if user is using one mirror registry like dockerproxy.com/docker.io/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review! This function ParseDockerRef() is just for backward compatibility. the function returns digested reference, e.g. docker.io/library/busybox:latest@sha256:xxxx will be returned as docker.io/library/busybox@sha256:xxxx.

Indeed, I guess we didn't have this scenario, so I have removed this function.

@zzjin
Copy link
Collaborator

zzjin commented Feb 10, 2023

@fengxsong Is this pr also resolves #2388 ?

@fanux
Copy link
Member

fanux commented Feb 10, 2023

Yes~

@cuisongliu cuisongliu linked an issue Feb 11, 2023 that may be closed by this pull request
@fanux fanux merged commit a791afb into labring:main Feb 11, 2023
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.

BUG: 镜像load后名字为localhost 使用离线安装时,sealos load 报错
4 participants