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

(maint) Remove warning about missing configuration file #91

Merged
merged 3 commits into from
Sep 3, 2020

Conversation

nwolfe
Copy link
Contributor

@nwolfe nwolfe commented Aug 21, 2020

Previously the CLI would emit a warning about missing configuration
file, but a configuration file does not appear to be a requirement, at
least based on the documentation and the fact that everything can be
specified directly on the command line.

This commit simply removes the warning.

Users will no longer get a warning when doing things like floaty --version, floaty help, and any other subcommand.

Reviewers

@puppetlabs/dio
@highb
@briancain

Previously the CLI would emit a warning about missing configuration
file, but a configuration file does not appear to be a requirement, at
least based on the documentation and the fact that everything can be
specified directly on the command line.

This commit simply removes the warning.

Users will no longer get a warning when doing things like `floaty
--version`, `floaty help`, and any other subcommand.
@nwolfe nwolfe requested review from briancain, highb and a team as code owners August 21, 2020 21:58
@nwolfe
Copy link
Contributor Author

nwolfe commented Aug 21, 2020

Happy to discuss/iterate on this but getting a WARNING on everything seems a bit off if users can specify everything on the CLI.

@nwolfe
Copy link
Contributor Author

nwolfe commented Aug 21, 2020

While we're thinking about the configuration file.... if we wanted to help users be aware of it....

Spitballing some ideas:

  • we could simply mention it in the floaty help text in the Description, so users don't need to notice it in the README, which isn't available via gem
  • we could add a config subcommand that checks/parses/validates/prints the file

@briancain
Copy link
Contributor

Hey @nwolfe ! That makes sense to me, especially if you can specify every option on the CLI without using a config file. I think as long as things fail gracefully when a config option is missing, we should be ok to remove this noisy warning! Also agreed that it would be good to mention something in the help text about where the config file should be.

So users can be aware of the configuration file since it will no
longer be emitted as a warning during CLI invocation.
@nwolfe
Copy link
Contributor Author

nwolfe commented Aug 21, 2020

it would be good to mention something in the help text about where the config file should be

I took a stab at this in 8341a5f

Help text now looks like:

$ floaty help
  NAME:

    floaty

  DESCRIPTION:

    A CLI helper tool for Puppet's vmpooler to help you stay afloat.

    Configuration may be placed in a ~/.vmfloaty.yml file.

  COMMANDS:

    completion Outputs path to completion script
    delete     Schedules the deletion of a host or hosts
    get        Gets a vm or vms based on the os argument
    help       Display global or [command] help documentation
    list       Shows a list of available vms from the pooler or vms obtained with a token
    modify     Modify a VM's tags, time to live, disk space, or reservation reason
    query      Get information about a given vm
    revert     Reverts a vm to a specified snapshot
    snapshot   Takes a snapshot of a given vm
    ssh        Grabs a single vm and sshs into it
    status     Prints the status of pools in the pooler service
    summary    Prints a summary of a pooler service
    token      Retrieves or deletes a token or checks token status

  GLOBAL OPTIONS:

    -h, --help
        Display help documentation

    -v, --version
        Display version information

    -t, --trace
        Display backtrace when an error occurs

Copy link
Contributor

@genebean genebean left a comment

Choose a reason for hiding this comment

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

LGTM

@nwolfe
Copy link
Contributor Author

nwolfe commented Aug 24, 2020

I think as long as things fail gracefully when a config option is missing, we should be ok to remove this noisy warning!

I'm finding a few commands that don't have all the CLI args they expect to find in the config file, such as delete and list.

Looking into adding a commit to update at least those.

To support usage without a configuration file.
@nwolfe
Copy link
Contributor Author

nwolfe commented Aug 24, 2020

Ok with the latest commit 9a44cc4 we can now get, list, and delete without a configuration file.

Get

$ floaty get --service=abs --user=nwolfe --token=**** centos-7-x86_64 --url=https://cinext-abs.delivery.puppetlabs.net/api/v2
Requesting VMs with job_id: 1598294267060.  Will retry for up to an hour.
Waiting 1 seconds to check if ABS request has been filled.  Queue Position: 148... (x1)
- fosterite-taunt.delivery.puppetlabs.net (centos-7-x86_64)

List

$ floaty list --active --token **** --user nwolfe --url https://cinext-abs.delivery.puppetlabs.net/api/v2 --service abs
Your VMs on cinext-abs.delivery.puppetlabs.net:
- [JobID:1598294267060] fosterite-taunt.delivery.puppetlabs.net (centos-7-x86_64) <allocated>

Delete

$ floaty delete 1598294267060 --token **** --user nwolfe --url https://cinext-abs.delivery.puppetlabs.net/api/v2 --service abs
Scheduled the following VMs for deletion:
- fosterite-taunt.delivery.puppetlabs.net

Copy link
Contributor

@briancain briancain left a comment

Choose a reason for hiding this comment

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

Thanks Nate!! 👍 as usual haven't really tested this, but the code looks good to me 😄 if someone wants to go through that process that would be cool!

@nwolfe
Copy link
Contributor Author

nwolfe commented Aug 24, 2020

@puppetlabs/dio anything more needed for merge?

If not merge away, or I can, or whatever the process is 😄

@briancain
Copy link
Contributor

FYI I am mostly stepping away as a merger or validater for this repo. I'm still fine with offering code reviews, but since I don't have access to vmpooler and all that anymore I won't be able to test PRs as much.

@nwolfe
Copy link
Contributor Author

nwolfe commented Sep 2, 2020

@highb
Copy link
Contributor

highb commented Sep 3, 2020

@genebean I think we're all good to merge this PR, if your testing indicates that it's good to go (I also don't have access to vmpooler or that PipelinesInfra repo so there's not much I can do to test it, either 🤷 ).

@genebean genebean merged commit 5a0640c into puppetlabs:master Sep 3, 2020
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

Successfully merging this pull request may close these issues.

4 participants