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

Public encoding.DecodeTypeMeta #52

Merged
merged 1 commit into from
May 16, 2024
Merged

Public encoding.DecodeTypeMeta #52

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions pkg/encoding/encoding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func Convert(codecs serializer.CodecFactory, inMediaType, outMediaType string, i
return nil, nil, fmt.Errorf("unsupported conversion: protobuf to kubernetes binary storage representation")
}

typeMeta, err := decodeTypeMeta(inMediaType, in)
typeMeta, err := DecodeTypeMeta(inMediaType, in)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -168,7 +168,7 @@ func tryFindJson(in []byte) (*json.RawMessage, bool) {
// DecodeSummary decodes the TypeMeta, ContentEncoding and ContentType fields from the 'Unknown'
// protobuf envelope of the given storage data.
func DecodeSummary(inMediaType string, in []byte, out io.Writer) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand why removing this function better? While DecodeSummary may not do much, it make sure there is in consistency in printing out summary for batch and non-batch mode. With this change, whenever there is some new meta field we want to display, the change has to be in 2 places, and that would be more error prone.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this function should only be used by cmd without exception, or just move it to cmd 😄.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't see much benefits of this refactoring. DecodeSummary is closely related to decode function itself, and can be used elsewhere in theory.
@jmhbnz WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

It's minor pr, though for me it's not particularly clear which high level problem this pr is trying to work towards which is why I have not already added review or approval for it.

I suggest we try focus community effort on progressing towards an agreed direction rather than refactor for the sake of it. Our challenge is obviously that auger has only recently come into sig-etcd and we have not created issues yet to establish direction to go in.

Perhaps we should step back a little from this code diff and try to get agreement on what problems we are wanting to solve so it's clear for future pull requests if they align with the direction we are trying to go in?

If project members or contributors can propose issues to start discussion it would be very helpful.

Copy link
Member Author

@wzshiming wzshiming May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with you. I've already made issue #3 describing what I wanted to achieve. which is directly edit the data from etcd, and using it just like kubectl.

Since the code changes are already a bit much, I'm trying to split it up as much for review.

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of this PR is to expose encoding.DecodeTypeMeta, make it can be used in other packages.
then see that encoding.DecodeSummary only applies to cmd, so it moves over to cmd

Copy link
Member

Choose a reason for hiding this comment

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

In the interest of getting this merged I suggest we limit the pr change to just making DecodeTypeMeta public. I am not in favor of the change to DecodeSummary.

Copy link
Member Author

@wzshiming wzshiming May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Done. change it as you say.

I am not in favor of the change to DecodeSummary.

And can you tell me why? As I understand DecodeSummary will never be used by others.

Copy link
Member

Choose a reason for hiding this comment

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

Personally because it is so early in lifecycle of auger within sig-etcd. We don't yet have regular releases occurring, or announcements of deprecations, or clear understandings of all use cases external to the project. For these reasons I am wary of removing a previously public function just for a very optional refactor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the clarification, I finally understand the difference in how we think about this, you see it as a legacy project that needs to ensure compatibility, and I see it as a new project, the history is actually in github.com/jpbetz/auger.

typeMeta, err := decodeTypeMeta(inMediaType, in)
typeMeta, err := DecodeTypeMeta(inMediaType, in)
if err != nil {
return err
}
Expand Down Expand Up @@ -220,8 +220,8 @@ func newCodec(codecs serializer.CodecFactory, typeMeta *runtime.TypeMeta, mediaT
return codec, nil
}

// getTypeMeta gets the TypeMeta from the given data, either as JSON or Protobuf.
func decodeTypeMeta(inMediaType string, in []byte) (*runtime.TypeMeta, error) {
// DecodeTypeMeta gets the TypeMeta from the given data, either as JSON or Protobuf.
func DecodeTypeMeta(inMediaType string, in []byte) (*runtime.TypeMeta, error) {
switch inMediaType {
case JsonMediaType:
return typeMetaFromJson(in)
Expand Down