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

Update value of localPath for composable packages #1154

Merged
merged 18 commits into from
Feb 1, 2023

Conversation

tworcester
Copy link
Contributor

@tworcester tworcester commented Jan 2, 2023

Description

Related Issue

Fixes #1153

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@jeff-mccoy
Copy link
Contributor

Thanks @tworcester we're waiting on #1173 to resolve issues around 3rd-party PRs

@jeff-mccoy
Copy link
Contributor

This is being weird, can you take a look at it again @tworcester?

@tworcester
Copy link
Contributor Author

This is being weird, can you take a look at it again @tworcester?

Sure thing, the test failures?

@tworcester
Copy link
Contributor Author

@jeff-mccoy Please try again, I believe I fixed the issue. I ran the tests locally but not the init-package 🤦

@tworcester
Copy link
Contributor Author

tworcester commented Jan 16, 2023

Do we want to provide the ability to respect a full file path? If people already using zarf have worked around this issue prior to this change, this would break them similar to what we saw above.

I think checking the provided localPath to see if it contains the zarf subcomponent folder already before adjusting would be defensive enough. Thoughts?

@jeff-mccoy
Copy link
Contributor

Yeah I think that sounds reasonable and would prevent this from being a breaking change, even though it's also a bug fix.

Copy link
Contributor

@jeff-mccoy jeff-mccoy left a comment

Choose a reason for hiding this comment

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

Add sanity check for existing behavior (component path in import path) to support existing broken behavior. Should leave a warning to the user noting this is incorrect.

@jeff-mccoy
Copy link
Contributor

Changing to draft to make tracking easier. Please complete the requested changes and then change back to ready for review, thanks!

@jeff-mccoy jeff-mccoy marked this pull request as draft January 21, 2023 04:51
@tworcester
Copy link
Contributor Author

Changing to draft to make tracking easier. Please complete the requested changes and then change back to ready for review, thanks!

Thank you! I likely won’t have time to get to this until next week. (Just wanted to drop a note so you know I haven’t dropped it)

@jeff-mccoy
Copy link
Contributor

not a problem at all, we may pick up the rest this week if we can squeeze it in, thanks!

@YrrepNoj YrrepNoj self-assigned this Jan 31, 2023
@jeff-mccoy jeff-mccoy marked this pull request as ready for review January 31, 2023 23:23
Copy link
Contributor

@YrrepNoj YrrepNoj left a comment

Choose a reason for hiding this comment

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

Eventually ™️, I think it would be a good idea to not even bother with the os.Stat() files from the parent directory perspective. Even though it's pretty unlikely, I think it's not that intuitive.

Of course since it's old behavior that we're trying not to break, it makes 110% sense to include in this PR for now!

@jeff-mccoy jeff-mccoy merged commit 54737ea into zarf-dev:main Feb 1, 2023
@tworcester
Copy link
Contributor Author

Thank you for wrapping this up, apologies for being absent!

Noxsios pushed a commit that referenced this pull request Mar 8, 2023
## Description

<!-- Please include a summary of the change. Any relevant motivation or
context is also helpful, as well as any dependencies that are required
for this change -->

## Related Issue

<!--- This project prefers to accept pull requests related to open
issues -->
<!--- If suggesting a new feature or change, please discuss it in an
issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps
to reproduce -->

Fixes #1153

## Type of change
- [x] Bug fix (non-breaking change which fixes an issue)

---------

Co-authored-by: Megamind <[email protected]>
Co-authored-by: Jonathan Perry <[email protected]>
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.

Helm Chart localPath not resolving properly when used with Composable Packages
4 participants