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

Make ytt portable to windows by changing the library key constant #524

Closed
joshrosso opened this issue Oct 27, 2021 · 5 comments
Closed

Make ytt portable to windows by changing the library key constant #524

joshrosso opened this issue Oct 27, 2021 · 5 comments
Assignees
Labels
enhancement This issue is a feature request priority/important-soon Must be staffed and worked on currently or soon.

Comments

@joshrosso
Copy link

Describe the problem/challenge you have

I'm using ytt, as a go dependency, in a project. The binary for this project is used on Mac, Linux, and Windows.

Due the the library reference key:

https://github.com/vmware-tanzu/carvel-ytt/blob/f53bcec371800411b4b2c4f5dde2b29b0610301c/pkg/cmd/template/data_values_flags.go#L211-L213

When absolute paths are used in Windows, the C:\\ is parsed as such due to the presence of :. This breaks ytt parsing when run on a Windows workstation.

Describe the solution you'd like

To workaround this, I created a fork, which does:

https://github.com/joshrosso/carvel-ytt/commit/74085add68cc7047676ee3ea3906702de4b9490b

And it works great. While I'm not suggesting you change your key to ^ like I did, I wonder if it could be changed or some logic put in place to make this portable to windows?


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@joshrosso joshrosso added carvel triage This issue has not yet been triaged for relevance enhancement This issue is a feature request labels Oct 27, 2021
stmcginnis pushed a commit to vmware-tanzu/community-edition that referenced this issue Oct 27, 2021
Due to an issue described in
carvel-dev/ytt#524, we need to rely
on a fork of `ytt` which changes an expected constant/key. Once this is
closed, we should go back to pointing at an official tag. Without this
in place, ytt parsing will fail when our binary is run on Windows.

Signed-off-by: joshrosso <[email protected]>
@pivotaljohn pivotaljohn added priority/important-soon Must be staffed and worked on currently or soon. and removed carvel triage This issue has not yet been triaged for relevance labels Oct 27, 2021
@pivotaljohn
Copy link
Contributor

Thanks for the report, @joshrosso.

While I'm not suggesting you change your key to ^ like I did, I wonder if it could be changed or some logic put in place to make this portable to windows?

Yes, sir: changing the token would be a breaking change that we'd like to avoid.

Having different path parsing strategies selected by OS seems more like the way to go.

@joshrosso
Copy link
Author

joshrosso commented Oct 27, 2021

Per request in slack, this is the function that would bubble up the error[0] on windows:

https://github.com/vmware-tanzu/community-edition/blob/40beed6d3632d9c0d089fc83ceb6e1840e608e92/cli/cmd/plugin/standalone-cluster/tkr/image.go#L157-L192

[0]:

Extracting data value from file: Expected library ref to start with '@'

@ChristianCiach
Copy link

ChristianCiach commented Nov 8, 2021

Java uses : on Linux and ; on Windows for its classpath notation. Maybe you should do the same.

See https://stackoverflow.com/a/55237119/1507544

joshrosso pushed a commit to vmware-tanzu/community-edition that referenced this issue Jan 18, 2022
Due to an issue described in
carvel-dev/ytt#524, we need to rely
on a fork of `ytt` which changes an expected constant/key. Once this is
closed, we should go back to pointing at an official tag. Without this
in place, ytt parsing will fail when our binary is run on Windows.

Signed-off-by: joshrosso <[email protected]>
@cari-lynn cari-lynn self-assigned this Feb 16, 2022
@cari-lynn
Copy link
Contributor

@joshrosso We are actively working on this now. Do you have a reproduction scenario so I can understand when this happens?

@cari-lynn
Copy link
Contributor

cari-lynn commented Mar 30, 2022

Hey this has been fixed in #609. It has been fixed by allowing : in file paths in o.DataValuesFlags.FromFiles or --data-values-file. This will allow Windows file paths to be used. This will be available once the next release is made (v0.41.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a feature request priority/important-soon Must be staffed and worked on currently or soon.
Projects
None yet
Development

No branches or pull requests

4 participants