- Notifications
You must be signed in to change notification settings - Fork928
feat: add support for template version messages in api and cli#8336
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
mtojek left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we want to support multi-line commit messages?
EDIT:
Don't know where are going to present it, but we can consider also regex.
| @@ -477,6 +477,7 @@ func TemplateVersion(t testing.TB, db database.Store, orig database.TemplateVers | |||
| CreatedAt: takeFirst(orig.CreatedAt, database.Now()), | |||
| UpdatedAt: takeFirst(orig.UpdatedAt, database.Now()), | |||
| Name: takeFirst(orig.Name, namesgenerator.GetRandomName(1)), | |||
| Message: orig.Message, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: what about testing? do we need a random message generated for unit tests or maybe benchmarks in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's a valid concern, although I didn't do it because I'm sure how we'd set an empty message if we usetakeFirst here on a randomly generated one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mafredri the emptytakeFirst is such an annoying problem 😢
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Good observation! Yes, I'd say we do. And that means I should add detection for newlines when informing about message too long. |
058504d to5bee51fCompareUh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Awesome feature!
This commit adds support for template version messages on the backend
and for the CLI commands
template createandtemplate push.Fixes the backend and CLI part of#3775