-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add LotMan configuration to the cache #889
Conversation
@cannon, let me know if you need anything else to define the Lots list in your object UI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First major issue. Because you branched off of add-cache-launcher at the beginning before it was finished, there are multiple duplicated commits at the beginning of this pull request. This will also cause conflicts with the finished version that is currently in the code. I suggest creating a new branch off of main and applying your specific commits (the last 9 as opposed to all 12) to the branch and recreating the PR otherwise merging this in will be a mess.
Since the cache launcher has been merged, it would be easier to rebase against main to forward commits to your new changes, but you might need to fix the merge conflicts in rebasing, if that's too much hassle, then Emma's idea should help |
If he had just branched off of my branch, I'd agree with you, but since I think he applied specific commits from my PR rather than branching, I don't this it's possible. This is one of the downsides of working with forked repos rather than on the main repo, it's not easy to branch off of other peoples branches. I just rebased and those 3 commits are still there. Without any of the potential later fixes I added. This will also add duplicate commits that say the same thing. So I think the answer is to drop those first three commits and then rebase. |
5e1dab7
to
ed1ef01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I will tack on a UI in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few clarification comments requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review focused on the token logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will approve once the parameter validation is fixed.
@CannonLock Is the parameter addition is something you'll need to handle, or can you give me a quick overview of how I can do it? |
@jhiemstrawisc Will push a commit on top of this in ~1 hours to add UI. |
- Add LotsField - Add DateTimeField - Add PathField
- Add Lots to verified object structures
- Pre-commit fixup
@jhiemstrawisc You will have to merge with main for the parameters check to pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving since Cannon's change is in
Okay, merging then! Thanks everyone 😄 |
This PR adds a variety of ways to interface with Lotman in the cache.
Note that as of my opening this PR, the CRUD actions exposed via gin don't have tests. While the token verification is very nuanced and undoubtedly contains mistakes, I propose we wait until the next release cycle to work on these tests because there's a cyclic import issue I need to figure out (launchers imports lotman to register everything, and lotman tests would need to import launchers to spin up the entire federation). For now, the API endpoints can be turned off, and the lots are configurable via the pelican config.
Closes #817