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

HTCONDOR-1731 Try replacing dots in token names with underscores #87

Merged

Conversation

jasoncpatton
Copy link

Workaround for https://opensciencegrid.atlassian.net/browse/HTCONDOR-1731

While adding support for underscores in URL schemes was laudable, there is still a problem parsing the (illegal) URLs upstream in HTCondor. Until we come up with a permanent solution, our workaround is to have users submit with a . in place of the _ (similar to how the client handles underscores internally) and have any plugins do the conversion from . to _ when doing token discovery.

I have refactored the HTCondor-specific token discovery code into its own function, which will search for both tokens with and without . replaced with _ in their filenames. The TestGetToken test was expanded to test URL schemes using both _ and . and token files named with both _ and ..

@jasoncpatton jasoncpatton requested a review from djw8605 October 5, 2023 13:07
@jasoncpatton
Copy link
Author

@djw8605 this is my first time writing in Go, so please be as harsh as possible on my code style and whatnot 🙂

@jasoncpatton jasoncpatton force-pushed the HTCONDOR-1731-dots-to-underscore branch 3 times, most recently from 37b4293 to cd1b54b Compare October 5, 2023 13:45
While adding support for underscores in URL schemes was laudible,
there is still a problem parsing the (illegal) URLs upstream in
HTCondor. Until we come up with a permanent solution, our workaround
is to have users submit with a "." in place of the "_" (similar to
how the client handles underscores internally) and have any plugins
do the conversion from "." to "_" when doing token discovery.

Here I have refactored the HTCondor-specific token discovery code
into its own function, which will search for both tokens with
and without "." replaced with "_" in their filenames. The
TestGetToken test was expanded to test URL schemes using both "_"
and "." and token files named with both "_" and ".".
@jasoncpatton jasoncpatton force-pushed the HTCONDOR-1731-dots-to-underscore branch from cd1b54b to 8930b9b Compare October 5, 2023 14:01
@jasoncpatton
Copy link
Author

@jhiemstrawisc reassigning review to you (same applies, any Go feedback welcome!)

Copy link

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

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

The approach looks solid, but my one minor nitpick is the mixing of camel and snake case. Elsewhere in the client we've tried to stick to camel case. Can you update variable names to match?

@jasoncpatton
Copy link
Author

The approach looks solid, but my one minor nitpick is the mixing of camel and snake case. Elsewhere in the client we've tried to stick to camel case. Can you update variable names to match?

Done for the discoverHTCondorToken function. I left the test code as is since it matches the other code blocks, despite the eyesores 🙂

Copy link

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

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

LGTM

@jhiemstrawisc jhiemstrawisc merged commit 766e7b4 into htcondor:main Oct 9, 2023
4 checks passed
@jasoncpatton jasoncpatton deleted the HTCONDOR-1731-dots-to-underscore branch October 9, 2023 14:20
jhiemstrawisc added a commit to jhiemstrawisc/pelican that referenced this pull request Oct 9, 2023
This PR brings a PR from OSDF-Client to Pelican that handles parsing token names
with `.`s in their name. For more information, see:
htcondor/osdf-client#87
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.

2 participants