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

Jump to bottom

Refactors Info and Get Command Interactive Mode #281

Merged
merged 2 commits into from
Jul 5, 2021
Merged

Refactors Info and Get Command Interactive Mode #281

merged 2 commits into from
Jul 5, 2021

Conversation

PuneetPunamiya
Copy link
Member

@PuneetPunamiya PuneetPunamiya commented Jun 25, 2021 •
edited
Loading

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2021
@tekton-robot tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 25, 2021
@PuneetPunamiya PuneetPunamiya force-pushed the refactor-interactive branch 2 times, most recently from 445ee42 to 4d26465 Compare June 25, 2021 16:17
@PuneetPunamiya
Copy link
Member Author

/retest

vinamra28
vinamra28 reviewed Jun 30, 2021
View reviewed changes
api/pkg/cli/utils/utils.go Outdated
Comment on lines 15 to 18
package utils

import (
"fmt"
Copy link
Member

Choose a reason for hiding this comment

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

instead of utils package I guess we can move this to options package. Also we can rename select_options package to options and move this over there. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hub cli commands already have a struct called options I think it will overlap with that 🤔

Copy link
Member

Choose a reason for hiding this comment

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

options package I am saying

api/pkg/cli/utils/utils.go Outdated
From string
Kind string
HubClient hub.Client
selectOption so.Options
Copy link
Member

Choose a reason for hiding this comment

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

The elements in the struct of so.Options are somewhat similar to the elements present in this struct so I guess we can merge both select_options.go and this one. WDYT?

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2021
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2021
@PuneetPunamiya PuneetPunamiya changed the title [WIP] Refactors Info and Get Command Interactive Mode Refactors Info and Get Command Interactive Mode Jul 5, 2021
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2021
vinamra28
vinamra28 reviewed Jul 5, 2021
View reviewed changes
Copy link
Member

@vinamra28 vinamra28 left a comment

Choose a reason for hiding this comment

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

a few comments otherwise looks good 👍

api/pkg/cli/cmd/get/get.go Outdated
@@ -211,22 +134,30 @@ func examples(kind string) string {
}

func (opts *options) GetResourceInfo() (string, error) {

Opts := so.Options{
Copy link
Member

Choose a reason for hiding this comment

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

should we rename this to askOpts or something better ?

Copy link
Member Author

Choose a reason for hiding this comment

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

May be only ask ??

Copy link
Member

Choose a reason for hiding this comment

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

I said askOpts just because we are settings the Options struct

api/pkg/cli/cmd/info/info.go Outdated
@@ -144,21 +142,29 @@ func (opts *options) run() error {
opts.hubClient = opts.cli.Hub()
var err error

Opts := so.Options{
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@PuneetPunamiya
Refactors info and get interactive mode
47059dc 
  - Adds a package by adding interactive mode
    functions for catalog, resourceName and
    version in order to reuse and make generic

Signed-off-by: Puneet Punamiya <[email protected]>
@vinamra28
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2021
@PuneetPunamiya
Copy link
Member Author

/retest

@sm43
Copy link
Member

sm43 commented Jul 5, 2021

/approve
/meow

@tekton-robot
Copy link

@sm43: cat image

In response to this:

/approve
/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sm43

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2021
@tekton-robot tekton-robot merged commit 5c698f1 into tektoncd:main Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinamra28 vinamra28 vinamra28 left review comments

@pratap0007 pratap0007 Awaiting requested review from pratap0007

@sthaha sthaha Awaiting requested review from sthaha

Assignees

@vinamra28 vinamra28

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PuneetPunamiya @vinamra28 @sm43 @tekton-robot