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

load configuration file with right ResourceLoader #701

Conversation

glours
Copy link
Contributor

@glours glours commented Oct 22, 2024

Detect and use the right ResourceLoader to load a config file and add it ConfigDetails struct.

LocalResourceLoader is always used as the last loader and should failed if it's not able to parse the configuration file.

This PR replace #592

@glours glours requested a review from ndeloof as a code owner October 22, 2024 13:05
@glours glours self-assigned this Oct 22, 2024
@glours glours requested a review from jhrotko October 22, 2024 13:06
@glours glours force-pushed the 554-use-resource-loader-to-ingest-compose-files branch from 7ba1cd6 to 95281eb Compare October 22, 2024 13:08
Copy link
Collaborator

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

LGTM, some minor comment


for i, p := range configFiles {
for _, loader := range opts.ResourceLoaders {
_, isLocalResourceLoader := loader.(localResourceLoader)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if local loader reject Accept it's highly probable it will also fail on Load, so why not just let it reject path and get this function return an error if no loader accepted the path ?

Copy link
Contributor Author

@glours glours Oct 22, 2024

Choose a reason for hiding this comment

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

Yes but you won't have the reason why.
What I could do is changing the Accept signature to return an error in addition of the boolean, WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the other option could be for localResource Loader to always accept path. The Load can return file not found error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll adapt the Accept function of resource loader

@glours glours force-pushed the 554-use-resource-loader-to-ingest-compose-files branch 2 times, most recently from aebbee9 to b5c8e72 Compare October 22, 2024 15:29
@glours glours requested a review from ndeloof October 23, 2024 09:32
@glours glours force-pushed the 554-use-resource-loader-to-ingest-compose-files branch 4 times, most recently from a41bb55 to 0f71bc7 Compare October 23, 2024 17:47
Mjaethers and others added 2 commits October 24, 2024 12:16
and pass workdir to LoadConfigFiles func to setup ConfigDetails

Signed-off-by: Guillaume Lours <[email protected]>
@glours glours force-pushed the 554-use-resource-loader-to-ingest-compose-files branch from 0f71bc7 to e16daf2 Compare October 24, 2024 10:16
@glours glours merged commit 74c1d59 into compose-spec:main Oct 24, 2024
8 checks passed
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.

3 participants