Skip to content

Feature/interactive mode #141

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
Merged
merged 17 commits into from
May 13, 2022
Merged

Feature/interactive mode #141

merged 17 commits into from
May 13, 2022

Conversation

@dritz01234
Copy link
Contributor

Added an interactive mode like it is described in #132.

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

Error in deno fmt. Please check the Action logs.

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

Error in deno lint. Please check the Action logs.

j0g3sc
j0g3sc requested changes Feb 4, 2022
View reviewed changes
Copy link
Contributor

@j0g3sc j0g3sc left a comment

Choose a reason for hiding this comment

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

Nice code quality! :)
I tested the interactive mode and it works nicely in general.
There are a few spelling mistakes, please review all user facing text.

Why I requested changes:
To me it was not clear why I need to choose a tag in the EXPLORE TENANTS WITH MISSING TAGS section. Please reformulate the text. Also instead of directly need to select one of the tenants for the detail view, we should really show the list of all those tenants that are lacking the chose tag ordered by costs, as this is the main feature here.

We can discuss face-to-face what i mean after today's meetings :)

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

Error in deno fmt. Please check the Action logs.

1 similar comment
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

Error in deno fmt. Please check the Action logs.

@dritz01234 dritz01234 requested a review from j0g3sc February 4, 2022 14:59
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

Error in deno fmt. Please check the Action logs.

tfelix
tfelix requested changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@tfelix tfelix left a comment

Choose a reason for hiding this comment

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

All in all a really clean and good code quality.

Base automatically changed from develop to main April 26, 2022 09:14
@j0g3sc j0g3sc force-pushed the feature/interactive-mode branch from acd7e7d to 320a21d Compare May 13, 2022 06:41
@github-actions
Copy link

Error in deno fmt. Please check the Action logs.

@github-actions
Copy link

Error in deno lint. Please check the Action logs.

@github-actions
Copy link

Error in deno lint. Please check the Action logs.

@github-actions
Copy link

Error in deno fmt. Please check the Action logs.

@j0g3sc j0g3sc requested a review from tfelix May 13, 2022 07:50
j0g3sc
j0g3sc reviewed May 13, 2022
View reviewed changes
Copy link
Contributor

@j0g3sc j0g3sc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

approved, but partly involved.

@j0g3sc j0g3sc requested review from j0g3sc and removed request for j0g3sc May 13, 2022 07:51
@j0g3sc j0g3sc requested review from j0g3sc and removed request for j0g3sc May 13, 2022 07:52
@github-actions
Copy link

Error in deno lint. Please check the Action logs.

@j0g3sc j0g3sc force-pushed the feature/interactive-mode branch from e58e737 to 2434e44 Compare May 13, 2022 07:55
tfelix
tfelix approved these changes May 13, 2022
View reviewed changes
Copy link
Contributor

@tfelix tfelix left a comment

Choose a reason for hiding this comment

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

Just two minor issues / comments that were not addressed. But looks good now.

src/commands/interactive/start-interactive-mode.ts

export async function startInteractiveMode(options: CmdGlobalOptions) {
console.clear();
const interactivehelp =
Copy link
Contributor

Choose a reason for hiding this comment

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

was not fixed

src/commands/interactive/untagged-tenants.ts
}
}
let runningInside = true;
while (runningInside) {
Copy link
Contributor

Choose a reason for hiding this comment

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

see this

@JohannesRudolph
Copy link
Member

I just gave this a test-drive on our various internal cloud environments and think this is a promising addition to collie. Great job on your first OSS contribution @dritz01234.

I will include this in the next "major" (we're still pre v1) release of collie, albeit flagging it as an experimental feature. This will need a bit of rework since that branch has diverged quite a bit from the current mainline, but I'll take care of merging that

@JohannesRudolph JohannesRudolph merged commit 87fac05 into main May 13, 2022
@JohannesRudolph JohannesRudolph deleted the feature/interactive-mode branch May 13, 2022 19:59
@JohannesRudolph JohannesRudolph mentioned this pull request May 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tfelix tfelix tfelix approved these changes

@j0g3sc j0g3sc j0g3sc approved these changes

@JohannesRudolph JohannesRudolph Awaiting requested review from JohannesRudolph

Assignees

No one assigned

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dritz01234 @JohannesRudolph @tfelix @j0g3sc