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.

Sign up for GitHub

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

Jump to bottom

new custom state check #784

Merged
merged 18 commits into from
Aug 17, 2023
Merged

new custom state check #784

merged 18 commits into from
Aug 17, 2023

Conversation

ruanxin
Copy link
Contributor

@ruanxin ruanxin commented Aug 7, 2023 •
edited
Loading

related issue: #196

@ruanxin ruanxin requested a review from a team as a code owner August 7, 2023 11:02
@kyma-bot kyma-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 7, 2023
@ruanxin ruanxin marked this pull request as draft August 7, 2023 11:02
@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2023
@ruanxin
Copy link
Contributor Author

ruanxin commented Aug 7, 2023

/test pull-lifecycle-mgr-build

@kyma-bot kyma-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 10, 2023
@ruanxin ruanxin marked this pull request as ready for review August 10, 2023 14:53
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2023
@ruanxin ruanxin added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2023
@nesmabadr nesmabadr mentioned this pull request Aug 11, 2023
Closed
2 tasks
@nesmabadr
Copy link
Contributor

  1. The documentation is missing, but we can tackle it in another issue
  2. CLI needs to support adding the customStateCheck field to the ModuleTemplate (Created this issue to track it )

nesmabadr
nesmabadr reviewed Aug 11, 2023
View reviewed changes
internal/manifest/ready_check.go Outdated
stateResult[stateCheck.MappedState] = stateFromCR == stateCheck.Value
}
}
return calculateFinalState(stateResult), true, nil
Copy link
Contributor

@nesmabadr nesmabadr Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't we add some validation in case all the jsonPaths are configured wrong, then the manifest would be in Warning state? because in this case, it will be Processing forever, although there is an error from the user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you define the "jsonPaths are configured wrong" a bit more concretely? If you mean the jsonPath can't be found, then it will fail in the stateFromCR, stateExists, err := unstructured.NestedString.

Copy link
Contributor

@nesmabadr nesmabadr Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yes I mean it can't be found. I tried a non-existing path, but the err was still nil here. Only the stateExists is false, and the stateFromCR is "" but no error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, good point, that's valid, which means

if !stateExists {
   continue
}

should not continue, it's better we indicate user this provided path not exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to check against the manifest CR creation timestamp to know when to put processing or warning state, please check here 2928408#diff-98067b6967c5fe86151bd1d8629e4107ff72feef3ac1a237c4b56e6eb2ad14e0R95-R99

@ruanxin
Copy link
Contributor Author

ruanxin commented Aug 14, 2023

/test pull-lifecycle-mgr-tests

@jeremyharisch jeremyharisch linked an issue Aug 14, 2023 that may be closed by this pull request
Introduce Custom StateCheck Paths in the Module Template #196
Closed
3 tasks
@ruanxin
Merge remote-tracking branch 'upstream/main' into new_custom_state_check
28e089a 
# Conflicts:
#	api/v1beta2/operator_annotations.go
@nesmabadr nesmabadr self-requested a review August 14, 2023 11:54
nesmabadr
nesmabadr previously approved these changes Aug 14, 2023
View reviewed changes
@kyma-bot kyma-bot added the lgtm Looks good to me! label Aug 14, 2023
@kyma-bot kyma-bot removed the lgtm Looks good to me! label Aug 16, 2023
@ruanxin
Copy link
Contributor Author

ruanxin commented Aug 16, 2023

/test pull-lifecycle-mgr-build

1 similar comment
@ruanxin
Copy link
Contributor Author

ruanxin commented Aug 16, 2023

/test pull-lifecycle-mgr-build

@kyma-bot kyma-bot added the lgtm Looks good to me! label Aug 17, 2023
@ruanxin ruanxin removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2023
@ruanxin
Copy link
Contributor Author

ruanxin commented Aug 17, 2023

/unhold

@kyma-bot kyma-bot merged commit e76fb55 into kyma-project:main Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nesmabadr nesmabadr nesmabadr approved these changes

Assignees

@nesmabadr nesmabadr

Labels
lgtm Looks good to me! size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce Custom StateCheck Paths in the Module Template
3 participants
@ruanxin @nesmabadr @kyma-bot