Skip to content

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

Merged
merged 13 commits into from
Jul 11, 2023

Conversation

mafredri
Copy link
Member

@mafredri mafredri commented Jul 6, 2023

This commit adds support for template version messages on the backend
and for the CLI commands template create and template push.

Fixes the backend and CLI part of #3775

@mafredri mafredri changed the title feat(cli,coderd): add support for template version messages feat: add support for template version messages in api and cli Jul 6, 2023
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mtojek mtojek left a 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,
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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 😢

@mafredri
Copy link
Member Author

mafredri commented Jul 6, 2023

Do we want to support multi-line commit messages?

Good observation! Yes, I'd say we do. And that means I should add detection for newlines when informing about message too long.

@mafredri mafredri force-pushed the mafredri/feat-add-template-push-message branch from 058504d to 5bee51f Compare July 6, 2023 14:29
@mtojek mtojek self-requested a review July 11, 2023 07:31
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Awesome feature!

@mafredri mafredri merged commit 75f62dc into main Jul 11, 2023
@mafredri mafredri deleted the mafredri/feat-add-template-push-message branch July 11, 2023 10:11
@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants