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.

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

chore: factorize condition result code out of resource plugins #1775

Merged

Conversation

dduportal
Copy link
Contributor

@dduportal dduportal commented Nov 23, 2023

This PR factorizes condition result code out of resource plugins:

  • Simplify code for plugins (easier for contributors to write/update resources as they have less things to handle and can focus on the resource code)
  • Centralize result object configuration (we can now change resource condition behavior in the core without impacting resources)
  • Separate condition result (true or false) from error (unexpected behavior such as network error)
  • Introduces the following behavioral changes (side effects of the PR):
    • githubrelease condition returns false but no error anymore when no upstream errors are triggered, with the same textual output as before.
    • gittag condition returns false but no error anymore when no upstream errors are triggered, with the same textual output as before.
    • golang condition returns false but no error anymore when no upstream errors are triggered, with the same textual output as before
    • gomod condition returns false but no error anymore when no upstream errors are triggered, with the same textual output as before
    • gomodule condition returns false but no error anymore when no upstream errors are triggered, with the same textual output as before
    • (nit) Homogenize methods to return an error (error.new() vs. fmt.Errorf())
    • (cleanup) removing obsolete "ConditionFromScm" tests

Test

To test this pull request, you can run the following commands:

make test

Additional Information

  • Unit test are not checking for the message returned by the Condition() function. It wasn't before this PR, so I chose to focus on the refactor to avoid too much changes in this PR

@dduportal dduportal force-pushed the chore/conditions-factorize-result-to-core branch from 273baf8 to 0abbc6f Compare November 23, 2023 20:26
@dduportal dduportal added condition Modify condition resource chore labels Nov 23, 2023
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal dduportal force-pushed the chore/conditions-factorize-result-to-core branch from a68e2b4 to 17cfd98 Compare November 23, 2023 20:47
@dduportal dduportal marked this pull request as ready for review November 23, 2023 21:00
@mcwarman
Copy link
Member

Going to continue to look more at the code as this is the sort of PR that takes multiple passes.

But I remember when I first looked to create a contribution I read this: https://github.com/updatecli/updatecli/blob/main/CONTRIBUTING.adoc#3-respect-the-contract

I realised it was out of date and moved on, in hind sight I should have looked to to update it. The whole page possibly needs a overhaul so might be out of scope for this change but contacts could be updated.

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal
Copy link
Contributor Author

Going to continue to look more at the code as this is the sort of PR that takes multiple passes.

But I remember when I first looked to create a contribution I read this: https://github.com/updatecli/updatecli/blob/main/CONTRIBUTING.adoc#3-respect-the-contract

I realised it was out of date and moved on, in hind sight I should have looked to to update it. The whole page possibly needs a overhaul so might be out of scope for this change but contacts could be updated.

Good point! I've tried to update a bit in 0d845e0, WDYT?

CONTRIBUTING.adoc Outdated Show resolved Hide resolved
Co-authored-by: Olivier Vernin <olivier@vernin.me>
@olblak
Copy link
Member

olblak commented Nov 24, 2023

I made another pass and I catch a few additional things. It's nice to see a lot of go code being cleanup

@olblak olblak added the cleanup label Nov 24, 2023
@dduportal dduportal merged commit 8649eb9 into updatecli:main Nov 25, 2023
6 checks passed
@dduportal dduportal deleted the chore/conditions-factorize-result-to-core branch November 25, 2023 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore cleanup condition Modify condition resource
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants