Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Split packages so we can extract gitops-toolkit #347

Merged
merged 5 commits into from
Aug 19, 2019
Merged

Conversation

luxas
Copy link
Contributor

@luxas luxas commented Aug 19, 2019

No description provided.

@luxas luxas requested a review from twelho as a code owner August 19, 2019 07:11
@luxas luxas added this to the v0.6.0 milestone Aug 19, 2019
@luxas luxas changed the title WIP: Changes for making the gitops toolkit work Split dependencies so we can refactor ignite into gitops-toolkit Aug 19, 2019
@luxas luxas added the kind/enhancement Categorizes issue or PR as related to improving an existing feature. label Aug 19, 2019
@luxas luxas changed the title Split dependencies so we can refactor ignite into gitops-toolkit Split packages so we can extract gitops-toolkit Aug 19, 2019
Copy link
Contributor

@twelho twelho left a comment

Choose a reason for hiding this comment

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

Looks great, just some nits.

func (s *serializer) encode(obj runtime.Object, mediaType string, pretty bool) ([]byte, error) {
info, ok := runtime.SerializerInfoForMediaType(s.codecs.SupportedMediaTypes(), mediaType)
if !ok {
return nil, fmt.Errorf("unable to locate encoder -- %q is not a supported media type", mediaType)
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace "--" with ": " for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not new code; just the same as before.

// decodable using the given serializer. However, all changes in the manifest directory, are also propagated to
// the structured data directory that's backed by the default storage implementation. Writes to this storage are
// propagated to both the manifest directory, and the data directory.
func NewTwoWayManifestStorage(manifestDir, dataDir string, ser serializer.Serializer) (*ManifestStorage, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TwoWay might be a bit confusing as this doesn't propagate data directory changes to the manifest directory (like the name implies). Is DataManifestStorage better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does propagate all the manifest that it knows about from before, so I think this is ok for now at least. We can talk about naming later.

@@ -42,8 +40,12 @@ func NewGenericWatchStorage(s storage.Storage) (WatchStorage, error) {
return nil, err
}

// TODO: Fix this
gvs := s.Serializer().Scheme().PreferredVersionAllGroups()
groupName := gvs[0].Group
Copy link
Contributor

Choose a reason for hiding this comment

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

OK for now

@luxas
Copy link
Contributor Author

luxas commented Aug 19, 2019

Merging to get going with the gitops-toolkit extraction

@luxas luxas merged commit 29d163e into master Aug 19, 2019
@luxas luxas deleted the gitops-toolkit branch August 26, 2019 17:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/enhancement Categorizes issue or PR as related to improving an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants