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

Lineage: First version of IsAppendOnly #123

Closed
wants to merge 2 commits into from

Conversation

AgnesToulet
Copy link
Contributor

This is a first basic implementation of IsAppendOnly(oldLineage, newLineage Lineage) that does string comparison.

Long-term, we probably want a more complex version that is less strict (allow authorized changes).

@AgnesToulet AgnesToulet added the enhancement New feature or request label Mar 31, 2023
lineage.go Show resolved Hide resolved
Copy link
Contributor

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

i realize this is still marked draft, but it does seem increasingly like we need to get some version of it as a prerequisite to other work.

IMO it's fine to merge this even if the comparison algorithm is flawed, so long as we're happy with the IsAppendOnly function signature.

return res, nil
}

func Equal(val1 cue.Value, val2 cue.Value) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need comment for exported func

@@ -268,3 +279,23 @@ type unaryConvLineage[T Assignee] struct {
func (lin *unaryConvLineage[T]) TypedSchema() TypedSchema[T] {
return lin.tsch
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Need comment for exported func. Probably worth noting in that comment that the comparison algorithm this currently uses is unsophisticated and subject to change

@@ -268,3 +279,23 @@ type unaryConvLineage[T Assignee] struct {
func (lin *unaryConvLineage[T]) TypedSchema() TypedSchema[T] {
return lin.tsch
}

func IsAppendOnly(oldLineage Lineage, newLineage Lineage) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is the signature we want. Even if we can't convey much in the error message right now, it'll be crucial that a failure here is able to provide line-level feedback on where the problem was.

Suggested change
func IsAppendOnly(oldLineage Lineage, newLineage Lineage) bool {
func IsAppendOnly(old, new Lineage) error {

@AgnesToulet
Copy link
Contributor Author

Closing in favor of #130

@AgnesToulet AgnesToulet deleted the lineage/is-append-only branch April 27, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants