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

feat(KNO-4537): Add "knock commit list" command in cli #256

Merged

Conversation

francoborr
Copy link
Contributor

Description

Adds support for using knock commit list command, which lists all commits for a given environment.
Allowed flags:
--promoted (or --no-promoted): Shows all promoted/unpromoted commits for a given environment.
--environment
--json
--limit
--after
--before

Tasks

KNO-4537

Screenshots

Copy link
Contributor

@thomaswhyyou thomaswhyyou left a comment

Choose a reason for hiding this comment

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

Overall this makes sense - left a few small comments and questions!

@@ -88,7 +88,8 @@ export default class CommitList extends BaseCommand<typeof CommitList> {
},
commit_message: {
header: "Commit message",
get: (entry) => entry.commit_message?.trim(),
get: (entry) =>
entry.commit_message ? entry.commit_message.trim() : "",
Copy link
Contributor Author

@francoborr francoborr Nov 7, 2023

Choose a reason for hiding this comment

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

Sometimes there are new lines after a given entry, which looks really bad in the terminal. After digging into this, I realized the solution is to delete all whitespaces in the commit message, which seems to generate new lines.

Before:

Screen Shot 2023-11-07 at 13 49 35

After:
Screen Shot 2023-11-07 at 13 55 17

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that makes sense.

One related question though — what happens with a really long commit message? In the screenshots above, it looks like we are doing truncation and I'm guessing that's automatically done by oclif?

Copy link
Contributor Author

@francoborr francoborr Nov 7, 2023

Choose a reason for hiding this comment

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

Yes initially I added a custom truncation function, but I removed it because oclif already uses one. And it works very well, the table takes all screen space available and the message truncates if it does not fit in the column.

Copy link
Contributor

Choose a reason for hiding this comment

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

And it works very well, the table takes all screen space available and the message truncates if it does not fit in the column.

Got it, thanks for clarifying. One issue here though is then, what happens if you want to see the entire identifier or the commit message without getting truncated? Just thinking out loud, we may need the commit get command for this?

Copy link
Contributor Author

@francoborr francoborr Nov 8, 2023

Choose a reason for hiding this comment

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

Good catch, we can do that in another PR if we feel like it.
And from what I've tested, the ID never gets truncated (because the length of the ID never changes and can't be that long i think)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, we can do that in another PR if we feel like it.

Yes that works!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, I don't think this should block the release of this feature, and is something we can add later. But we can discuss more of this with the team if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I were to make a call I personally would get that in before releasing the feature because 1) it's straight forward enough, but also because 2) right now the commit message is the only way to reason about what it is that you are trying to promote and not being able to see the entire message feels a hindrance to using the feature.

Maybe there's a way to force oclif not to truncate and print out the whole commit message but then that may introduce another issue of the table formatting getting messed up.

Anyways open to discussion for sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I said this because this feature was planned to be shipped in the next changelog tomorrow, as we thought there was not going to be more changes. I can try to get it done before that, but with that + reviewing it (and we are still in code review in other PR'S) we probably don't get to the timeline.

I can propose moving this to next changelog as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha — yeah let's focus on getting the current outstanding PRs turned around and merged, and then we can chat about the changelog and when it goes out.

Copy link
Contributor

@thomaswhyyou thomaswhyyou left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Left one follow up question.

@@ -88,7 +88,8 @@ export default class CommitList extends BaseCommand<typeof CommitList> {
},
commit_message: {
header: "Commit message",
get: (entry) => entry.commit_message?.trim(),
get: (entry) =>
entry.commit_message ? entry.commit_message.trim() : "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that makes sense.

One related question though — what happens with a really long commit message? In the screenshots above, it looks like we are doing truncation and I'm guessing that's automatically done by oclif?

Copy link
Contributor

@thomaswhyyou thomaswhyyou left a comment

Choose a reason for hiding this comment

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

Looks good to go!

@francoborr francoborr merged commit 8664e98 into main Nov 9, 2023
@francoborr francoborr deleted the franco-kno-4537-cli-add-knock-commit-list-command-in-cli branch November 9, 2023 14:54
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.

2 participants