-
Notifications
You must be signed in to change notification settings - Fork 34
Support mutation M.M-1 #2454
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
base: main
Are you sure you want to change the base?
Support mutation M.M-1 #2454
Conversation
🔍 Preview links for changed docs |
reakaleek
left a comment
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 🚀
But let's wait for @Mpdreamz feedback because he has all the context about this feature.
| [Display(Name = "M.M")] MajorMinor, | ||
| [Display(Name = "M+1")] IncreaseMajor, | ||
| [Display(Name = "M.M+1")] IncreaseMinor, | ||
| [Display(Name = "M.M-1")] DecreaseMinor, |
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.
Might as well implement decreasemajor? :)
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.
Not sure about the use cases or benefits, but we can easily implement it, yeah. I'll add it to the 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.
Added to the PR. We'd return 8.0.0 now no matter what 9.x.x release we are currently in.
In the future, whenever we are on 10.x.x it would return 9.0.0.
If major is 0 (0.y.z) it would return the original value, same as with the M.M-1.
As soon as we decide what exactly to return in these corner cases I will adapt the 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.
Aye thats what i was thinking!
Actually, I think we should error if the user tries to decrease a 0 minor version. |
I thought about that, but I thought it was a bit risky considering this is dynamic... in the future whenever 10.0.0 is released we don't want our docs to fail building because M-1 starts failing, or? Maybe indeed we want :) but then we should change the docs until 10.1.0 is released.... I was doubting between returning an empty string or the original value. If you think it's better to return an error that's totally ok. |
|
I agree it's better to anchor it to 0 and dont return an error. |
In the worst case this will show an incorrect version and we won't even be aware of it. |
@Mpdreamz , do you mean to normalize it to 0 in a similar way than the +1 operations? So, instead of returning the exact original value:
In such case I'd do M-1 in a similar way:
Did you mean this approach or do you prefer to return the "original value" in these special cases? |
This would render
Is that an error or acceptable? I am not sure its an error @reakaleek, wether its acceptable I'll defer to the writers @eedugon @elastic/docs-tech-leads
This relates to: #2111 @eedugon can you discuss this with @shainaraskas ? |
What would happen if we were at |
|
Yes. But is this the expected value? Shouldnt it be 9.x.x? Probably depends on the context. But IMO, this leaves too much space for unintended versions.. and if we just use a fallback to |
|
Ahh no
To me thats the path of least surprise. |
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 think a compromise would be to create a hint in the case we need to fallback to 0.
IMO, the fallback can lead to unexpected version rendering
and a wrong version has worse consequences than a build error. But with a hint are at least aware of it.
P.S.
IMO we should also document the behaviour
|
@Mpdreamz , your formula is OK, but it's not the formula I need to use: Your formula: That's actually a But I'm going to use Because I only need Anyway I'd say it's all OK.
|
|
@reakaleek , the behavior is documented
Do you mean in the code? THere's only one line with the payload and at the moment it's pretty simple: We need to clarify if it's ok to return the original value when Whatever you prefer (cc: @Mpdreamz ) |

We need this functionality to keep track of the versions supported by default on Elastic Cloud Hosted, which are:
This PR focuses on the first item.
I'd like {{version.stack | M.M-1 | M.M }} to render to 9.1 if version.stack is for example 9.2.3.
If minor version is 0 we return the original value without deducing anything, to avoid a negative number.
PS - I've used cursor to implement this.
This functionality would be required by elastic/docs-content#4607