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

[API breaking change] add prefix change indicator #4403

Merged
merged 6 commits into from
Oct 25, 2022

Conversation

eden-ohana
Copy link
Contributor

@eden-ohana eden-ohana commented Oct 19, 2022

closes #4068
documentation will be added as part of #4070

I'm open to better suggestions for a name to indicate prefix change

@eden-ohana eden-ohana added the include-changelog PR description should be included in next release changelog label Oct 19, 2022
@eden-ohana eden-ohana self-assigned this Oct 19, 2022
@eden-ohana eden-ohana added the team/versioning-engine Team versioning engine label Oct 19, 2022
Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

Thanks! Some questions & comments:

  1. Does the UI support it out of the box? I think we should at least introduce new colouring to changed prefixes to differentiate them from changed objects.
  2. We need some testing (new or modify existing) that validates the new behaviour.
  3. I think we should make the documentation changes in this PR, Docs: Explain metadata operations costs and affects better #4070 is a whole different issue (although it involves docs too..).

@@ -14,6 +14,8 @@ func transformDifferenceTypeToString(d catalog.DifferenceType) string {
return "changed"
case catalog.DifferenceTypeConflict:
return "conflict"
case catalog.DifferenceTypePrefixChanged:
return "prefix changed"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return "prefix changed"
return "changes under prefix"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@johnnyaug
Copy link
Contributor

@itaiad200 - please, no more colors in the diff screen. The UI shows a folder icon, indicating that this is a prefix and not a single object. I would argue that this enough.

@eden-ohana
Copy link
Contributor Author

@itaiad200

  1. Currently, The UI shows the same color for prefix as for change. I don't think we should add another color for it, maybe add text with "changes under this prefix" but I'm not sure it's necessary because it has a change color and folder icon.
  2. Done
  3. There isn't an explanation on the docs about the current diff behavior, on Docs: Explain metadata operations costs and affects better #4070 I'm adding a diff section on the docs that we can explain there in detail. unless you have an idea where to document it for now.

@eden-ohana eden-ohana requested a review from itaiad200 October 24, 2022 13:48
@itaiad200
Copy link
Contributor

@eden-ohana
Copy link
Contributor Author

image

Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

LGTM
Please change the hover message on the directory symbol

@eden-ohana eden-ohana merged commit 39c5d81 into master Oct 25, 2022
@eden-ohana eden-ohana deleted the 4068/diff-prefix-changed branch October 25, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog team/versioning-engine Team versioning engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prefix Change indicator for Diff should change to reflect prefix change
3 participants