-
Notifications
You must be signed in to change notification settings - Fork 887
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
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 use takeFirst
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 empty takeFirst
is such an annoying problem 😢
Good observation! Yes, I'd say we do. And that means I should add detection for newlines when informing about message too long. |
058504d
to
5bee51f
Compare
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 create
andtemplate push
.Fixes the backend and CLI part of #3775