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: create config url should be stored in /flat TDE-1321 #841

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

paulfouquet
Copy link
Collaborator

Motivation

The key output by get-location does not end with a trailing /. The create-config task should manage the path within the bucket correctly.

Modification

Add a / between the key and the flat in the artifact path for the config-url.

Checklist

  • Tests updated
  • Docs updated
  • Issue linked in Title

l0b0
l0b0 previously approved these changes Nov 3, 2024
amfage
amfage previously approved these changes Nov 3, 2024
@@ -650,7 +650,7 @@ spec:
path: '/tmp/cogify/config-url'
s3:
bucket: '{{inputs.parameters.bucket}}'
key: '{{inputs.parameters.key}}flat/config-url'
key: '{{inputs.parameters.key}}/flat/config-url'
Copy link
Member

Choose a reason for hiding this comment

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

should this other part of the code change too?

          - 'config'
          - '{{ inputs.parameters.location }}flat/'

how many other places are expecting this key to be a folder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

{{ inputs.parameters.location }} is already having a trailing /. I think "location" speaks well by itself being a path and should have a trailing /.

Copy link
Collaborator Author

@paulfouquet paulfouquet Nov 3, 2024

Choose a reason for hiding this comment

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

I agree with you, we need a something to parse and concatenate paths withing workflow files. This looks like doing what we want: https://masterminds.github.io/sprig/url.html - maybe we can try work with it if it's available within Argo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did few tests using a couple of sprig functions:
{{=sprig.clean(sprig.join("/", [tasks[get-location].outputs.parameters.location, "flat/"]))}}

Problem is that the path we're trying to clean is a URL and is not handled properly by the clean function which removes the double / in s3:// -> s3:/

@paulfouquet paulfouquet changed the title fix: create config url should be stored in /flat fix: create config url should be stored in /flat TDE-1321 Nov 5, 2024
@paulfouquet paulfouquet dismissed stale reviews from amfage and l0b0 via e9393f6 November 5, 2024 22:40
@paulfouquet
Copy link
Collaborator Author

@schmidtnz suggested to trimSuffix / of the location and key paths before concat them to avoid having to know if these path ends with a / or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants