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

feat: add resource controller #30

Merged
merged 20 commits into from
Oct 22, 2024
Merged

feat: add resource controller #30

merged 20 commits into from
Oct 22, 2024

Conversation

frewilhelm
Copy link
Contributor

@frewilhelm frewilhelm mentioned this pull request Sep 27, 2024
Closed
@frewilhelm frewilhelm self-assigned this Sep 27, 2024
@frewilhelm frewilhelm force-pushed the resource-controller branch 6 times, most recently from f172316 to c703991 Compare October 2, 2024 16:02
@frewilhelm frewilhelm force-pushed the resource-controller branch 2 times, most recently from 675d3e5 to c15f788 Compare October 7, 2024 16:07
@frewilhelm
Copy link
Contributor Author

Both tests are failing because go.mod uses a local version of ocm that provides signing.VerifyResourceDigestByResourceAccess.

The function is already merged in ocm (see https://github.com/open-component-model/ocm/pull/943/files) but a new release providing the function is missing.

Nevertheless, the PR is ready for review (but won't be merged until a new ocm release is available and our GH actions are reporting no errors)

@frewilhelm frewilhelm marked this pull request as ready for review October 7, 2024 16:17
@frewilhelm frewilhelm force-pushed the resource-controller branch from c15f788 to 725d4ef Compare October 7, 2024 16:22
jakobmoellerdev
jakobmoellerdev reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

Mostly nits, logic itself I believe works in "golden path". Not sure about the tons of error cases though because they are all not retryable (maybe I read that wrong).

Regarding the performance of the controller I have serious doubts under scale, but I would need to benchmark it with some big descriptors to be sure.

@frewilhelm frewilhelm force-pushed the resource-controller branch 2 times, most recently from dd7c7ac to 95ad964 Compare October 8, 2024 15:02
@frewilhelm frewilhelm changed the title Implement resource controller feat: add resource controller Oct 9, 2024
@frewilhelm frewilhelm force-pushed the resource-controller branch 2 times, most recently from f1ac7b9 to dc6d54e Compare October 15, 2024 15:21
@frewilhelm frewilhelm marked this pull request as draft October 15, 2024 15:22
@frewilhelm frewilhelm marked this pull request as ready for review October 21, 2024 11:51
This was referenced Oct 21, 2024
Closed
Open
Closed
Open
jakobmoellerdev
jakobmoellerdev requested changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but missing the watch config

frewilhelm and others added 2 commits October 22, 2024 13:27
jakobmoellerdev
jakobmoellerdev approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

LGTM, time to start working on TODOs

@jakobmoellerdev jakobmoellerdev merged commit 68c9e38 into main Oct 22, 2024
3 checks passed
@jakobmoellerdev jakobmoellerdev deleted the resource-controller branch October 22, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabianburth fabianburth fabianburth approved these changes

@jakobmoellerdev jakobmoellerdev jakobmoellerdev approved these changes

@Skarlso Skarlso Awaiting requested review from Skarlso

Assignees

@frewilhelm frewilhelm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Resource Controller
4 participants
@frewilhelm @Skarlso @fabianburth @jakobmoellerdev