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

The road to 0.2.0 #158

Closed
rhuss opened this issue Jun 2, 2019 · 2 comments
Closed

The road to 0.2.0 #158

rhuss opened this issue Jun 2, 2019 · 2 comments

Comments

@rhuss
Copy link
Contributor

rhuss commented Jun 2, 2019

@sixolet, @cppforlife, all: Sorry for the many PRs that I opened recently. They all address 0.2.0 issues though and the good news is, I think we are feature complete now wrt/ to 0.2.0.

The not-so-good news is that all those implementations are currently hanging in PRs so there is still some integration work to do. Also, there is some slight overlap (not surprisingly for a still young project) between PR, so it might make sense to work through them in a particular order.

Let me suggest a possible way to go through the PRs. Please don't get me wrong, I don't want to push too hard, but I think we have an excellent chance to release 0.2.0 very soon if we touch down with the pending PRs. They don't get better the older they get (it's not like wine ;).

#66 - Introduce subpackages

This PR should be the first one to integrate so that the other PRs could be rebased on this, as this introduces quite some structural (i.e. moving files) changes, which makes it hard to merge. Alternatively, this PR could also go last so that only one PR (this one) needs to be adapted (or more or less redone). Regardless of how I believe this one of the most essential PRs and should also be tagged for milestone 0.20. I reserve some time tomorrow for a review, would be super awesome if we could get it merged until the next WG meeting.

#134 - Add a GvkUpdate mixing method to client

This PR is done, and all requested changes have been made. I agree that this approach is still a bit tedious to call that update manually, but better than hiding in the ResourcePrinters (as suggested in #153). It's a right solution and could be an intermediate step to a dedicate knative-client internal access client (like indicated in #75), but that could be left for 0.3.0. [fixes #133]

#155 - Lemonade, step 1 (update only thos fields which are received)

Again a PR which is good to merge. It's limited in scope as it only affects only 'kn service update' (but also creeps into the tests). I think I have caught all cases, but a review would be excellent. [fixes #126, #144]

#156 - Wait for service until ready after kn service create

Needs a review. There is one minor gotcha with kn service create --force, but this could be fixed in a later step. Otherwise, there is one open question about the final console output (e..g whether to also print the service URL) [fixes #54]

#157 - minScale/maxScale/target support

Done and needs review. It's based on top #156 so should be integrated after it. If #156 doesn't make, it should be quite straight forward to port it to master again. This feature is not planned for 0.2.0 but I think it should be added (as it's done already ;-) [fixes #146]

#139 - Remove unused --config option

The PR is straight forward, but there is still an open question to @sixolet why it was introduced at the first time. It's not much code, we could readd it later if we should introduce a local configuration file. [fixes #138]

#150 - Support for an argument kn service get and kn revision get

This is to complete the functionality of a simple file on the list returned by kn service get (like for kubectl get which allows the name of thre resource to fetch). To be honest, I'm not sure about the naming (get vs. list as discussed in the WG meeting and revived in #128). But let's put this discussion to #128 where I will summarize my thoughts on naming)

The remaining two 0.2.0 issues are also fixed as part of #75 (list revisions #127, ), and in a not yet-PRed commit and that small, that should be easily incorporated. [fixes #143]


That brings me to PR #75 (kn service describe) which suggest different flavours of output for a kn service describe. Unfortunately, there has not yet been a discussion where we want to go to. The current suggestion is to have three levels of output to address a broad audience: text only (a la kubectl), with colours (as knctl does is a bit) and a very condensed view with using the full UTF range of icons. Every level can be switched on/off individually and also combined. On a second axis, the detail level can be set to either a short summary presentation or full details (like the whole env of all revisions attached to a service).

This and also the kind of data to show (e.g. should traffic via routes that involve revisions connected to the specified service also been demonstrated in kn service describe ?) needs to be decided. It's all on the plate, but I don't mind to reduce and adapt it to what we think makes sense. However, it would be very awesome if I could get some feedback and decision what to do with #75. It's not well tested mainly because tests will be about the actual output we still have to decide upon. But let's continue the discussion over there at #75 (if you want to)


Sorry again if this all sounds a bit like pushing (it is of course ;), but definitely not in anger), it would be awesome if we could advance a bit faster (otherwise its eventually all about resolving PR conflicts as long as the project is still that small that we are in danger to tread on one another's toes). I'm happy to help where I can (my Github/Prow karma is limited though, so I need your help for making the real things happen ;-)

Said all that, I'm super happy to be able to contribute and I believe that we are on the right track to make an awesome cli for an awesome project ;-)

(updated with ✅ for everything which has been already merged)

@maximilien
Copy link
Contributor

maximilien commented Jun 3, 2019

Great summary @rhuss. I am happy to have #66 updated before or after. Whatever is the path of least resistance. If after I clearly will have more work :( than if before :) but happy to take one for the team.

I will address some pending comments but we should discuss this tomorrow. I added an entry on the agenda.

Hopefully we can tackle that and plugins requirements? If not, we should delay plugins since this is more urgent IMO.

@sixolet
Copy link
Contributor

sixolet commented Jul 11, 2019

0.2 is out!

@sixolet sixolet closed this as completed Jul 11, 2019
navidshaikh pushed a commit to navidshaikh/client that referenced this issue Nov 7, 2019
Runs cross platform build to ensure all the checks pass
coryrc pushed a commit to coryrc/client that referenced this issue May 14, 2020
* Readd dummy.go to the root dir: Because we `dep` the root dir of `test-infra`, it must contain go code.
* Ignore `kubetest` failures in `e2e-tests.sh`: Cluster teardown failures shouldn't affect the test results.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants