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

feat(bundle): optionally displays labels associated to a bundle #342

Closed
wants to merge 1 commit into from

Conversation

fredbi
Copy link
Contributor

@fredbi fredbi commented Dec 20, 2019

Signed-off-by: Frederic BIDON frederic@oneconcern.com

Note to reviewers

This I consider an early experiment to gauge competing ideas about how to deal with CLI features that pertain to the "porcelain" layer.

While I am all in for some DSL approach that would eventually result in neater code, I have difficulties figuring out how it would actually look like.

This PR plays here the devil's advocate and is my 2 cents contribution to this interesting debate about implementing porcelain, with some code-grounded facts. No big deal if this PR is discarded....

I wanted to figure out a more accurate idea about how much of code/complexity such user requirements actually entail when implemented with the basic tooling the CLI is currently providing.

Regarding expected results, I followed the approach recommended by @ransomw1c in #235 (";" separators) and did not try to over-engineer it with some asynchronous fetching of labels.

@fredbi fredbi marked this pull request as ready for review December 30, 2019 20:10
@fredbi fredbi force-pushed the fix-235 branch 2 times, most recently from 0a1c82e to 6828b1a Compare January 3, 2020 09:28
@ransomw1c
Copy link
Contributor

ransomw1c commented Jan 9, 2020

just reading the note to reviewers.

the thing about calling this a "porcelain layer" change is that it's actually using some additional "plumbing," core.ListLabels, that wasn't previously consumed by the cmd/ package.

if this deduped and re-used the list labels functionality such that the bundle list and list labels relied on literally the exact same data path to retrieve labels, then i think it would be clearer why the requirement could be implemented independent of adding the additional getLabels porcelain to the pkg/ ListLabels plumbing.

however, as i say, i don't think this a pure porcelain change. it's like if we were shipping async def someFunc() in some alternative universe where the cli was implemented via python bindings, then started shipping def someFuncSync(). even if the data paths of someFunc and someFuncSync are pretty much the same under the hood, that's not an api guarantee. from downstream, they're separate functions. or at least the water is murkier than it would be in the situation described in the previous paragraph.


possible takeaway of the preceding rant: refactor core.ListLabels into cmd/?

@fredbi
Copy link
Contributor Author

fredbi commented Jan 10, 2020

Re remark on python bindings

@ransomw1c on the slack channel I posted a proposal about figuring out some python bindings (I've done that in the past, binding python 2 and 3 to a go lib, so I could do this again). This could really be a nice thing, but I didn't receive any comment on that post.

@ransomw1c
Copy link
Contributor

quick note on collective opinions here: after discussion with Joao last week, i suppose it's better understood that my remarks about shipping a Python api to access Datamon functionality in addition to the cli api are purely motivated by attempting to fulfill feature requests related to the cli api without turning cmd/datamon/cmd into spaghetti, and of course we'll all agree that we don't want spaghetti.

at the same time, there are ways to keep cmd/datamon/cmd maintainable without negotiating cli feature requests into a Python api – refactoring cobra use or using a different cli lib altogether to name a couple.

supposing tentative plans for the couple weeks (see Jira RES-1809) occur, one possible scenario – a likely one, even, imo – is that we'll continue to cruise toward cutting v2 with a mild amount of tech debt in datamon/cmd and meanwhile condense hack/fuse-demo/wrap_datamon.sh (note that this prototype we're about to ship to production is still in fuse-demo/, btw) into a (hopefully much terser) Python script.

so i'll propose to continue worrying about the maintainability of cmd/datamon/cmd, except only to first try to make it more maintainable using golang only. meanwhile, there will be some Pythonic faceting of the binary for a totally different raison, the maintainability of the sidecar. and, imo, writing the Pythonic faceting for that purpose could also help get clarity on using python as a means to make the desktop CLI more maintainable.

* fixes #235

Signed-off-by: Frederic BIDON <frederic@oneconcern.com>
@kerneltime
Copy link
Contributor

We can park the branch for now. We need to think more about this feature.

@fredbi
Copy link
Contributor Author

fredbi commented Feb 7, 2020

@kerneltime sure. I wanted to close it to. Ransom did rightfully point out that this should go as porcelain. Organizing the porcelain layer is going to be the big code shuffle that will come together with the python SDK.

@fredbi fredbi closed this Feb 7, 2020
@fredbi fredbi deleted the fix-235 branch April 2, 2020 09:12
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.

Datamon bundle list should include labels
3 participants