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 yaml resource source filepath #1187

Merged
merged 6 commits into from
Feb 28, 2023
Merged

Conversation

olblak
Copy link
Member

@olblak olblak commented Feb 28, 2023

While automating a file update, I spot a regression where the yaml resource wouldn't use the correct filepath when used with scm.

Test

I updated the e2e tests to catch this issue but it's worth mentioning they will only pass once the current pullrequest is merged on the main branch

Additional Information

Tradeoff

Potential improvement

  • A source pipeline shouldn't set by default the current working directory. It's very confusing in resource plugins.
    pwd, err := os.Getwd()
    I would need to investigate the initial intend and then review all plugins as I first noticed this behavior in the cargo package plugin and now in the yaml

Signed-off-by: Olblak <me@olblak.com>
@olblak olblak added the resource-yaml Resource of kind YAML label Feb 28, 2023
@ghost
Copy link

ghost commented Feb 28, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@olblak
Copy link
Member Author

olblak commented Feb 28, 2023

Also while running some tests, I noticed that a key not found in a yaml will not return an error
https://github.com/updatecli/updatecli/blob/main/pkg/plugins/resources/yaml/source.go#L63L67

I can't remember if it's on purpose. @dduportal @hervelemeur maybe you recall some decision around that

@lemeurherve
Copy link
Member

lemeurherve commented Feb 28, 2023

Also while running some tests, I noticed that a key not found in a yaml will not return an error
main/pkg/plugins/resources/yaml/source.go#L63L67

I can't remember if it's on purpose. @dduportal @hervelemeur maybe you recall some decision around that

No idea, sorry. There was some comments about another key in my PR, but unrelated to that one: #650 (comment)

@olblak olblak merged commit 7bdbe14 into updatecli:main Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resource-yaml Resource of kind YAML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants