feat: add to and from flags to FOASCLI sunset diff command#1243
feat: add to and from flags to FOASCLI sunset diff command#1243lovisaberggren merged 6 commits intomainfrom
Conversation
lovisaberggren
requested review from
andmatei,
andreaangiolillo,
drinkbird,
maks-m-mongo-leaf,
matt-condon,
saisundar and
wtrocki
| } | ||
| } | ||
|
|
||
| return out, nil |
There was a problem hiding this comment.
Do we support open intervals here (either of the date is not provided)? If this code returns a nil slice, would something break?
Do we have tests to cover ☝️ ☝️ ?
There was a problem hiding this comment.
Good Qs!
Do we support open intervals here (either of the date is not provided)?
Hmm, we do however I don't think it's currently working as expected. When only one of the flags is provided, for example only from, I'd expect the behaviour to be "any date from the from date forward" and the other way around. I'll add tests to cover this case to ensure it works as expected
If this code returns a nil slice, would something break?
It wouldn't, it would either be an empty slice or one populated with values. err can be an error or nil
There was a problem hiding this comment.
Made a small adjustment to make it work as I described above, and added tests to cover those cases. Let me know what you think! Ty
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
diffsInRange returns error but I don't think any branch ever sets it — the time.Parse failures just fall back to time.Time{}. Woud it be cleaner to drop the return type and the matching if err != nil plumbing in findDiffs / Run(), or actually propagate a wrapped error when the date string is malformed?
There was a problem hiding this comment.
Good catch, this is probably left from some changes I made.
I think we would want to distinguish between the date beling Nil/empty, and invalid. In the case of invalid we should probably return an error instead of silently fail. I'll make some changes 👍
| for _, d := range diffs { | ||
| baseSunsetDate, err := time.Parse("2006-01-02", d.BaseSunsetDate) | ||
| if err != nil { | ||
| baseSunsetDate = time.Time{} // zero value for time if parsing fails |
There was a problem hiding this comment.
Nit thing I wanted to check — if someone ships a spec with a malformed sunset date, this coerces to time.Time{} and silently drops it from the drift report. Since the whole point of the followup GH action is to alert on drift, a bad date is exactly what we'd want to catch. Maybe gaurd empty-string explicitly and return an error on genuinely malformed non-empty values?
There was a problem hiding this comment.
We do check that the sunset dates are valid in MMS CI, so it would be caught before the change is shipped to OpenAPI. As per my comment above I'll update to error for invalid dates anyways, if it for some reason would slip through
| return yamlData, nil | ||
| } | ||
|
|
||
| func (o *DiffOpts) validate() error { |
There was a problem hiding this comment.
Quick one: validate() doesn't check that from <= to. An inverted range (someone swaps the flags in the workflow YAML) will just return an empty diff, which looks the same as "no drift" and the CI job passes silently. Worth an early error like --from must not be after --to?
There was a problem hiding this comment.
Good point, I'll add a check 👍
| return out, nil | ||
| } | ||
|
|
||
| func isDateInRange(date, from, to *time.Time) bool { |
There was a problem hiding this comment.
Should this zero-time guard live in diffsInRange instead? isDateInRange is shared with sunset list, which doesn't produce time.Time{} sentinels — so the two commands' semantics are now coupled through a case only diff creates. Also noticed there's no list_test.go change exercising the new branch.
There was a problem hiding this comment.
Makes sense, I'll adjust that to be handled in diffsInRange
3 participants
Proposed changes
Adds
toandfromflags to thefoascli sunset diffcommand to filter out endpoints with sunset dates that are outside the range. The flags can also be provided alone/independently.The output contains the endpoint and it's sunset date for the base spec and the comparison spec. The endpoint is included in the output if one of the sunset dates are within the provided range.
Adding this feature to be able to create a GH action diff report for sunset drifts where one of the dates are set to "within 3 weeks from now".
Jira ticket: CLOUDP-397437