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

warn user remote resource is disabled #11051

Merged
merged 3 commits into from
Oct 18, 2023
Merged

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Sep 29, 2023

What I did
as user forgot (?) to enable git/oci remote resource support (still experimental) compose tries to resolve a local path from URL. This is obvious something is wrong, but the way to get this fixed isn't.

this PR let the remote resource loader detect it should load a remote resource, but report an explicit error and way to fix it.

@ndeloof ndeloof requested review from a team, nicksieger, StefanScherer, ulyssessouza, glours, milas and laurazard and removed request for a team September 29, 2023 07:30
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Attention: 50 lines in your changes are missing coverage. Please review.

Comparison is base (a345515) 57.55% compared to head (599e4b2) 57.73%.
Report is 14 commits behind head on main.

❗ Current head 599e4b2 differs from pull request most recent head 3d0207e. Consider uploading reports for the commit 3d0207e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11051      +/-   ##
==========================================
+ Coverage   57.55%   57.73%   +0.18%     
==========================================
  Files         129      129              
  Lines       11237    11245       +8     
==========================================
+ Hits         6467     6492      +25     
+ Misses       4141     4127      -14     
+ Partials      629      626       -3     
Files Coverage Δ
cmd/compose/compose.go 70.36% <100.00%> (+2.34%) ⬆️
pkg/remote/git.go 8.80% <0.00%> (+7.10%) ⬆️
pkg/remote/oci.go 12.50% <0.00%> (+10.11%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM when the lint issue will be fixed

@glours glours enabled auto-merge October 18, 2023 10:42
@@ -114,51 +114,51 @@ func (g ociRemoteLoader) Load(ctx context.Context, path string) (string, error)
return "", err
}

s, err2 := g.pullComposeFiles(ctx, local, composeFile, manifest, ref, resolver)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could also rename variable err

@glours glours merged commit 5e1d3f2 into docker:main Oct 18, 2023
22 checks passed
@glours
Copy link
Contributor

glours commented Oct 18, 2023

Sorry I see your comment too late 😬

@ndeloof ndeloof deleted the warn_remote_disabled branch December 20, 2023 13:55
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