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

Initial implementation for the Reconcile Logic #17

Merged
merged 1 commit into from
Jan 11, 2025

Conversation

gdasson
Copy link
Member

@gdasson gdasson commented Jan 4, 2025

This PR is an intial implementation of the design mentioned here

The code is now in working state and able to create an etcd cluster. However, the code will continue to be refined and made more production ready as we review and incorporate feedback.

@ahrtr
Copy link
Member

ahrtr commented Jan 4, 2025

@gdasson Thanks for the PR.

Could you please resolve the comments and ensure the PR is in the best shape you think before marking it as ready for review and also remove "draft" from the title? I will take a second round of review once you mark it as ready for review.

@ahrtr ahrtr marked this pull request as draft January 4, 2025 14:50
@gdasson gdasson changed the title Initial Draft for the Reconcile Logic Initial implementation for the Reconcile Logic Jan 5, 2025
@gdasson gdasson marked this pull request as ready for review January 6, 2025 03:27
@ahrtr
Copy link
Member

ahrtr commented Jan 6, 2025

@jmhbnz @ivanvc Do you know why workflow checks are not triggered in this PR? cc @hakman

Also it'd be great if we could add more e2e test case as mentioned in #19. Of course, in followup PRs.

@ahrtr ahrtr requested review from hakman and justinsb January 6, 2025 12:46
@ahrtr
Copy link
Member

ahrtr commented Jan 7, 2025

@hakman @justinsb @jmhbnz @ArkaSaha30 Please take a look at this PR. It will be the base for the following PRs. Thanks.

cc @ivanvc

Also I am most concerned about the e2e test as mentioned in #17 (comment)

@jmhbnz
Copy link
Member

jmhbnz commented Jan 7, 2025

@jmhbnz @ivanvc Do you know why workflow checks are not triggered in this PR? cc @hakman

Probably the GitHub actions bug we see from time to time, we may need to close and re-open this pr for it trigger. I will migrate the existing workflows to prow longer term.

/ok-to-test
/retest

@jmhbnz jmhbnz closed this Jan 7, 2025
@jmhbnz jmhbnz reopened this Jan 7, 2025
@ivanvc ivanvc mentioned this pull request Jan 7, 2025
3 tasks
@ahrtr
Copy link
Member

ahrtr commented Jan 8, 2025

@gdasson please rebase this PR, thx

@ahrtr
Copy link
Member

ahrtr commented Jan 10, 2025

@gdasson please rebase this PR and let's merge this PR. Also squash the commits (I can also do it for you if you have any difficulties). I will create more following tasks.

@gdasson
Copy link
Member Author

gdasson commented Jan 10, 2025

@ahrtr : I'll work on it today. I had a busy couple of days, I apologize for not responding earlier.

@gdasson gdasson force-pushed the feature/initial_draft2 branch from 3511661 to 6c46e88 Compare January 11, 2025 05:15
@gdasson gdasson force-pushed the feature/initial_draft2 branch from 6c46e88 to 2906c2c Compare January 11, 2025 05:22
@gdasson
Copy link
Member Author

gdasson commented Jan 11, 2025

@ahrtr : Done. Please review. Thanks.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the nice work!

Will plan/raise more tasks next week.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, gdasson

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants