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

🌱 Introduce ReconcileError with Transient and Terminal Error type #787

Merged
merged 1 commit into from
Jan 22, 2025
Merged

🌱 Introduce ReconcileError with Transient and Terminal Error type #787

merged 1 commit into from
Jan 22, 2025

Conversation

Sunnatillo
Copy link
Member

This PR:

Introduces ReconcileError with Transient and Terminal Error type
Removes RequeueAfterError (as it was depricated in CAPI and CAPM3)

ReconcileError represents an generic error of Reconcile loop. errorType indicates what type of action is required to recover. It can take two values:
a. Transient - Can be recovered , will be requeued after.
b. Terminal - Cannot be recovered, will not be requeued.

When error is not ReconcileError it will be returned to Reconciler.

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 12, 2024
@Sunnatillo
Copy link
Member Author

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

Rozzii
Rozzii reviewed Dec 13, 2024
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/test metal3-ubuntu-e2e-integration-test-main

Thanks for the contribution, nice work in general but I have a question below.

ipam/reconcile_error.go
@@ -0,0 +1,85 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

I assume from the license message, that this file and the related test file were copied.
Is there a possibility to find these Error types in some library that we could pull in as a dependency instead of duplicating this code?

If this is not copied but written by you, then my question is that why are you adding the K8s author copyright?

Copy link
Member

Choose a reason for hiding this comment

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

I know other files have this copyright here, I will start a discussion with the community related to that also.

Copy link
Member Author

@Sunnatillo Sunnatillo Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I have copied from capm3. I am pretty much sure I have written those code in capm3. What would be proper copyright in this case?

Copy link
Member

Choose a reason for hiding this comment

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

We have used at least # Copyright 2023 The Metal3 Authors. elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed now. I used 2024 as PR is opened in 2024

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @Sunnatillo !

tuminoid
tuminoid reviewed Jan 14, 2025
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

We did similar for CAPM3 too, some time back right?

/cc @kashifest @lentzi90

@mquhuy
Copy link
Member

mquhuy commented Jan 17, 2025

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2025
@tuminoid
Copy link
Member

/hold
until the copyright is corrected.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2025
@Sunnatillo
Introduce ReconcileError with Transient and Terminal Error type
fa6581d 
Remove RequeueAfterError(depricated in CAPM3)

Signed-off-by: Sunnatillo <sunnat.samadov@est.tech>
@Sunnatillo Sunnatillo force-pushed the replace-requeafterError-type/sunnat branch from 2a930b2 to fa6581d Compare January 21, 2025 08:14
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2025
@Sunnatillo
Copy link
Member Author

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

@Sunnatillo
Copy link
Member Author

/test metal3-ubuntu-e2e-integration-test-main

1 similar comment
@Sunnatillo
Copy link
Member Author

/test metal3-ubuntu-e2e-integration-test-main

lentzi90
lentzi90 reviewed Jan 21, 2025
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/approve
Related to this, I guess we should also think about following CAPI by using conditions instead of ErrorMessage, right?

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2025
@tuminoid
Copy link
Member

/retest

Rozzii
Rozzii reviewed Jan 22, 2025
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2025
@Sunnatillo
Copy link
Member Author

/approve Related to this, I guess we should also think about following CAPI by using conditions instead of ErrorMessage, right?

yes, I will checl what beta2 is bringing and trying to align with it

@tuminoid
Copy link
Member

This is currently held for copyright issue, which is now fixed. But then

yes, I will checl what beta2 is bringing and trying to align with it

Are you going to change this PR still, or merge this and then change it later?

Unhold if latter.

@Sunnatillo
Copy link
Member Author

This is currently held for copyright issue, which is now fixed. But then

yes, I will checl what beta2 is bringing and trying to align with it

Are you going to change this PR still, or merge this and then change it later?

Unhold if latter.

No, this is different task.
/unhold

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2025
@metal3-io-bot metal3-io-bot merged commit 2707785 into metal3-io:main Jan 22, 2025
13 checks passed
@metal3-io-bot metal3-io-bot added this to the IPAM - v1.10 milestone Jan 22, 2025
@metal3-io-bot metal3-io-bot deleted the replace-requeafterError-type/sunnat branch January 22, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rozzii Rozzii Rozzii left review comments

@lentzi90 lentzi90 lentzi90 left review comments

@tuminoid tuminoid tuminoid left review comments

@kashifest kashifest Awaiting requested review from kashifest

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
IPAM - v1.10
Development

Successfully merging this pull request may close these issues.
6 participants
@Sunnatillo @mquhuy @tuminoid @metal3-io-bot @lentzi90 @Rozzii