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

feature: addition of list-commits and show-commit commands #10867

Merged
merged 13 commits into from Nov 12, 2019

Conversation

nammn
Copy link

@nammn nammn commented Nov 6, 2019

To change/add

  • sort list-commits desc
  • time format in human readable format (4h ago...)
  • no commits feedback

Description of change

adds:

  • commits
  • show-commit <number>
    commands

changes:

  • renames feature flag from generations to branches

QA steps

preparation

  • bootstrapped controller
export JUJU_DEV_FEATURE_FLAGS=generations
juju deploy ubuntu
juju add-branch bla
juju add-branch blub
juju track bla ubuntu
juju config ubuntu hostname=random
juju commit bla

list-commits test

❯ juju commits
Commit	Committed at              	Committed by	Branch name
3     	07 Nov 2019 14:41:41+01:00	admin       	bla    

should not show anything other than bla, because blub did not get committed

It should not show aborted branches as well!

no branches committed yet

~/golang/src/github.com/juju/juju list-commit
❯ juju commits                      
Commit	Committed at	Committed by	Branch name

--> might want to change that to a proper message

show-commits test

> juju show-commit 3

branch:
  bla:
    applications:
    - application: ubuntu
      units:
        tracking:
        - redis/0
      config:
        hostname: bla
committed-at: 08 Nov 2019 10:14:10+01:00
committed-by: admin
created: 08 Nov 2019 10:13:04+01:00
created-by: admin

err case:

## Documentation change
- rename feature flag from generations to branches

❯ juju show-commit 10 
ERROR generation_id 10 in model "default" not found

Bug reference

@nammn nammn force-pushed the list-commit branch 2 times, most recently from c2e1ef9 to 0406a80 Compare November 7, 2019 14:00
apiserver/params/params.go Outdated Show resolved Hide resolved
cmd/juju/model/listcommits.go Show resolved Hide resolved
cmd/juju/model/listcommits.go Outdated Show resolved Hide resolved
cmd/juju/model/listcommits.go Show resolved Hide resolved
cmd/juju/model/showcommit.go Outdated Show resolved Hide resolved
cmd/juju/model/showcommit.go Outdated Show resolved Hide resolved
cmd/juju/model/showcommit.go Outdated Show resolved Hide resolved
@nammn nammn force-pushed the list-commit branch 2 times, most recently from 6c55254 to ac2ff53 Compare November 7, 2019 17:08
@nammn nammn marked this pull request as ready for review November 8, 2019 09:25
@nammn nammn changed the title list and show commit command: initial command addition addition of: list-commits and show-commit commands Nov 8, 2019
@nammn nammn force-pushed the list-commit branch 2 times, most recently from 37efe3e to 05062c2 Compare November 8, 2019 10:53
@nammn
Copy link
Author

nammn commented Nov 8, 2019

!!build!!

windows failed...

Copy link
Member

@SimonRichardson SimonRichardson left a comment

Choose a reason for hiding this comment

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

There are a few nit issues (they're not required, just if you've got time).

cmd/juju/model/listcommits.go Outdated Show resolved Hide resolved
cmd/juju/model/listcommits.go Outdated Show resolved Hide resolved
cmd/juju/model/showcommit.go Outdated Show resolved Hide resolved
cmd/juju/model/showcommit.go Outdated Show resolved Hide resolved
cmd/juju/model/showcommit_test.go Outdated Show resolved Hide resolved
state/modelgeneration.go Outdated Show resolved Hide resolved
nam added 2 commits November 8, 2019 17:14
@mitechie
Copy link
Contributor

mitechie commented Nov 8, 2019

Thanks for this, really fun to play with. Some feedback

I think you're correct on the "no commits made yet" feedback message. We do this in other places, like if you don't have an active model or controller. In storage it's "No storage to display" so how about sticking with that and doing "No commits to list" (without the headings) when there are none.

For the list-commits can we sort them desc please as the more recent history at the top is useful. e.g. list-commits | head with the headings vs if you have it at the bottom you've got to let it scroll and lose the headings in the output. I think over time we might make this more interesting and only go back so far without a --all or something but for now I think just sorting reverse would be good.

For the listing committed at dates, can we do a condensed date format. I'm trying to see if we have a standard for this elsewhere, but I'm tempted to run with what we use in the "juju list-users" output for "Date created" as that's a pretty nice human readable time there. Having the detailed date in show-commit is perfect though. The other way to go would be just a simpler date but then we get into format debates and the like.

Finally, with us asking folks to play with this I think we need to change the feature flag name to not be "generations" but "branches" to match what the users are interacting with. Any change that's an easy drive-by here?

…output, commits feedback

featureflags: rename flag from generations to branches
Copy link
Member

@manadart manadart left a comment

Choose a reason for hiding this comment

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

Logic all looks good. Just some mechanical stuff I'd like to see.

api/modelgeneration/modelgeneration.go Outdated Show resolved Hide resolved
api/modelgeneration/modelgeneration.go Outdated Show resolved Hide resolved
apiserver/facades/client/modelgeneration/interface.go Outdated Show resolved Hide resolved
core/model/generation.go Outdated Show resolved Hide resolved
Copy link
Member

@manadart manadart left a comment

Choose a reason for hiding this comment

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

Looks great.

@nammn
Copy link
Author

nammn commented Nov 12, 2019

!!build!!

@nammn
Copy link
Author

nammn commented Nov 12, 2019

thanks for the review of this chunk!

$$merge$$

@jujubot jujubot merged commit ee8ec9c into juju:develop Nov 12, 2019
@nammn nammn changed the title addition of: list-commits and show-commit commands feature: addition of list-commits and show-commit commands Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants