rework the commitments data model and API #383
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
majewsky
force-pushed
the
commitment-rework
branch
6 times, most recently
from
c3de719
to
70fbd42
Compare
- "confirm_after" becomes "confirm_by" in a move that anticipates an upcoming semantic change in the commitment state machine. This change will come later, but since I'm doing a DB migration anyway, I'm including it here. - "requested_at" becomes "created_at" because the "request" wording only applies to completely fresh commitments. Later, we will add workflow for transferring, splitting and converting commitments. All of these will have a creation timestamp without that creation referring to a request. - I'm adding some fields to identify who created a commitment, since this information may be valuable, but surfacing it from the CADF audit log is usually quite tedious. This includes a change to the vendored go-api-declarations copy that does not exist in this form in that module's upstream repo. All the changes to go-api-declarations to this feature branch will be bundled into a single commit in go-api-declarations in the end to avoid a bunch of useless intermediate releases on that upstream repo.
The test for this is quite incomplete because a full test requires reworking the POST .../new endpoint, which I will do in the next commit. This one is large enough as it is.
- If `confirm_by` is absent, the commitment must be immediately confirmed upon creation. Insufficient committable capacity is an error. - If `confirm_by` is present, the commitment will be confirmed after that timestamp as soon as committable capacity is available. - To avoid surprises for the users, `expires_at` counts from the moment when the commitment could first be confirmed, instead of from the moment of confirmation. This is only a difference for commitments with delayed confirmation: ``` expires_at = confirmed_at + duration # before expires_at = confirm_by + duration # after ```
majewsky
force-pushed
the
commitment-rework
branch
from
383c506
to
6438c36
Compare
VoigtS
requested changes
Dec 6, 2023
VoigtS
requested changes
Dec 6, 2023
VoigtS
approved these changes
Dec 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on feedback from the Demand Management folks, and my own thoughts based on that feedback. Roughly ordered by complexity:
requested_atfield tocreated_at(in the future, there will be several workflows to create commitment objects that are not really requests)min_confirm_datein commitment configconfirm_afterwhile renaming it toconfirm_byconfirm_bycan be chosen by the userconfirm_bycan only be created if they can be confirmed immediately (hence why we need the aforementioned dry run ability for a clean UI workflow)confirm_bywill haveexpires_at = confirm_by + durationinstead ofexpires_at = confirmed_at + duration(i.e. if we take longer to provide the requested capacity, the customer shall not be bound to pay for it longer than they anticipated when requesting)confirm_byneed to haveconfirm_by >= min_confirm_date(hence why this config attribute needs to be surfaced in the API)This includes changes to the vendored go-api-declarations copy that does not exist in this form in that module's upstream repo. All the changes to go-api-declarations to this feature branch will be bundled into a single commit in go-api-declarations in the end to avoid a bunch of useless intermediate releases on that upstream repo.Done.