Skip to content
This repository was archived by the owner on Jan 28, 2025. It is now read-only.

Remove EntitySource - #108

Merged
perdasilva merged 5 commits into
operator-framework:mainfrom
perdasilva:rewrite/entitysrc-per
Aug 15, 2023
Merged

Remove EntitySource#108
perdasilva merged 5 commits into
operator-framework:mainfrom
perdasilva:rewrite/entitysrc-per

Conversation

@perdasilva

@perdasilvaperdasilva commented Jul 18, 2023

Copy link
Copy Markdown
Contributor

This PR replaces #95 by rebasing it and fixing merge conflicts. The intent of this PR is to remove the concepts of EntitySource and Entity from Deppy. The VariableSource interface can be implemented with access to specific backend clients (e.g. registry, kube client, etc.) in order to provide their Variables.

Closes#97

@perdasilva
perdasilva requested a review from a team as a code ownerJuly 18, 2023 14:39
@codecov

codecovBot commented Jul 18, 2023

Copy link
Copy Markdown

Codecov Report

Merging #108 (54375ee) into main (4c15c99) will increase coverage by 3.54%.
The diff coverage is 80.00%.

@@ Coverage Diff @@## main #108 +/- ##
==========================================
+ Coverage 61.90% 65.45% +3.54% 
==========================================
Files 16 11 -5 Lines 567 495 -72 ==========================================
- Hits 351 324 -27 + Misses 198 152 -46 - Partials 18 19 +1 
Files ChangedCoverage Δ
cmd/dimacs/cmd.go0.00% <0.00%> (ø)
cmd/dimacs/dimacs_constraints.go67.85% <100.00%> (+67.85%)⬆️
pkg/deppy/solver/solver.go82.85% <100.00%> (-0.48%)⬇️

... and 1 file with indirect coverage changes

@ncdc

ncdc commented Jul 18, 2023

Copy link
Copy Markdown
Member

Is there some background on this change?

@ncdc

ncdc commented Jul 18, 2023

Copy link
Copy Markdown
Member

I take it this is for #97? And it presumably replaces #95?

@perdasilva
perdasilvaforce-pushed the rewrite/entitysrc-per branch from a585358 to 32471fbCompareAugust 2, 2023 11:20
@perdasilva

Copy link
Copy Markdown
ContributorAuthor

Sorry @ncdc - I started this just before I left for holidays. Yeah, the idea was to take Varsha's PR, rebase, fix and put it back here. I'll fill out the description and tie it to a ticket.

@perdasilva
perdasilvaforce-pushed the rewrite/entitysrc-per branch 11 times, most recently from e3c8255 to beb7312CompareAugust 7, 2023 16:34

@m1kolam1kola left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The change looks good to me apart from removal of rand.Seed.

Updating operator-controller will be fun. And I'll be missing predicates.

Comment threadcmd/sudoku/sudoku.go Outdated
// adapted from: https://github.com/go-air/gini/blob/871d828a26852598db2b88f436549634ba9533ff/sudoku_test.go#L10
variables := make(map[deppy.Identifier]*input.SimpleVariable, 0)
inorder := make([]deppy.Variable, 0)
rand.Seed(time.Now().UnixNano())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We are not on Go 1.20 yet in Deppy:

go1.19

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just merged the Go 1.20 bump. Need to rebase here now, it looks like.

@tmshort

Copy link
Copy Markdown

Requires another rebase...

@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2023
varshaprasad96and others added 5 commits August 11, 2023 11:37
Removes Entity and EntitySource. The solver takes in
the Variable Source directly. The constraints, filters, sort
or any transformation should be performed on variables before
providing it to solver.
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
@perdasilva
perdasilvaforce-pushed the rewrite/entitysrc-per branch from beb7312 to 54375eeCompareAugust 11, 2023 09:38
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2023

@m1kolam1kola left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me!

@perdasilva
perdasilva merged commit f4b138b into operator-framework:mainAug 15, 2023
@joelanfordjoelanford mentioned this pull request Aug 21, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the EntitySource API and replace it with the VariableSource API

7 participants

@perdasilva@ncdc@tmshort@m1kola@joelanford@openshift-merge-robot@varshaprasad96