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

init bundle diff #218

Merged
merged 1 commit into from
Aug 28, 2019
Merged

init bundle diff #218

merged 1 commit into from
Aug 28, 2019

Conversation

ransomw1c
Copy link
Contributor

the bundle diff part of #214

pkg/core/bundle_unpack.go Outdated Show resolved Hide resolved
@ransomw1c ransomw1c marked this pull request as ready for review June 24, 2019 06:35
@ransomw1c ransomw1c force-pushed the i214-initdiff--wip branch 2 times, most recently from 815ffff to 824c5bb Compare June 24, 2019 22:35
@ransomw1c ransomw1c force-pushed the i214-initdiff--wip branch 3 times, most recently from a02ff06 to 7158328 Compare June 26, 2019 22:52
@ransomw1c ransomw1c changed the title [wip] init bundle diff init bundle diff Jun 26, 2019
@ransomw1c
Copy link
Contributor Author

since #220 is a dep, i'll resolve conflicts after it lands

@ransomw1c ransomw1c force-pushed the i214-initdiff--wip branch 3 times, most recently from 5ca407f to 917c7bc Compare July 29, 2019 23:33
BundleDescriptor: model.BundleDescriptor{},
}
err := core.PopulateFiles(context.Background(), &bundle)
bundle := core.New(core.NewBDescriptor(),

Choose a reason for hiding this comment

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

What is the rationale behind this type of syntactic change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since the cmd/datamon/cmd package uses core.New (admittedly sugar), the main idea here is consistency.

now, the core.New itself is something that we could view as idiomatic, a sort-of borrowed idiom from Java, etc. where constructor functions are used. golang idioms are liable to change over time, yet in no small part because i buy into the utility of type inference, i suppose the constructor idiom is a helpful one: we get to treat the return values of constructors as semi-loosely (i.e. duck) typed.

bundleDescriptorBuffer, err := storage.ReadTee(ctx,
bundle.MetaStore, model.GetArchivePathToBundle(bundle.RepoID, bundle.BundleID),
bundle.ConsumableStore, model.GetConsumablePathToBundle(bundle.BundleID))
func setBundleIDFromConsumableStore(ctx context.Context, bundle *Bundle) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not getBundleIDFromPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've added a getBundleIDFromPath function that returns a bundle id given the path to a bundle descriptor within the bundle (and the empty string when given another path such as a filelist path) and used it within setBundleIDFromConsumableStore.

it occurs to me that what getBundleIDFromPath could also mean is to get the bundle id from a path to a bundle on the local filesystem, except i suppose keeping the abstraction away from the local filesystem via storage is a desirable abstraction.

also, i could inline the contents of setBundleIDFromConsumableStore since it's only used once .. or not: i'm ambivalent re. the aesthetics of whether to maintain setBundleIDFromConsumableStore as a separate function or not.

@ransomw1c ransomw1c force-pushed the i214-initdiff--wip branch 3 times, most recently from 069eb3f to 74917d6 Compare August 23, 2019 19:10
Signed-off-by: Ransom Williams <rwilliams@oneconcern.com>
@ransomw1c ransomw1c merged commit b1a6e74 into master Aug 28, 2019
@ransomw1c ransomw1c deleted the i214-initdiff--wip branch August 28, 2019 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants