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.

Sign up for GitHub

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

Jump to bottom

feat: use customizable message templates for PR comments #175

Merged
merged 4 commits into from
Apr 7, 2022
Merged

feat: use customizable message templates for PR comments #175

merged 4 commits into from
Apr 7, 2022

Conversation

koenigle
Copy link
Member

@koenigle koenigle commented Mar 30, 2022 •
edited

Allows one to define custom message templates in the .gitops.config.yml which are used for the PR comments
If no templates are specified, default messages are used

Example .gitops.config.yml

applicationName: my-application
messages:
  created: "Created preview for ${GIT_HASH}. You can access your application here: https://prefix-${PREVIEW_HOST}/some-cool-path"
  updated: "Updated preview to ${GIT_HASH}. You can access your application here: https://prefix-${PREVIEW_HOST}/some-cool-path"
  uptodate: "Your application is already up to date with ${GIT_HASH}. You can access your application here: https://prefix-${PREVIEW_HOST}/some-cool-path"

christiansiegel
christiansiegel requested changes Mar 30, 2022
View reviewed changes
Copy link
Member

@christiansiegel christiansiegel left a comment

Choose a reason for hiding this comment

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

Thanks, great idea 👍
Could you please also update the docs? (https://github.com/baloise/gitopscli/blob/master/docs/includes/preview-configuration.md)

gitopscli/gitops_config.py Outdated Show resolved Hide resolved
gitopscli/gitops_config.py Outdated
@@ -358,6 +407,18 @@ def __parse_v2(self) -> GitOpsConfig:
return GitOpsConfig(
api_version=2,
application_name=self.__get_string_value("applicationName"),
messages_created_template=self.__get_string_value_or_default(
"messages.created",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe previewConfig.messages.created would be a better place in the yaml structure? https://baloise.github.io/gitopscli/commands/create-pr-preview/#gitopsconfigyaml

Copy link
Member Author

Choose a reason for hiding this comment

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

For me the previewConfig.* path looked more like technical configuration of the preview environment whereas the messages are rather general information for the GitOps Bot, thus I thought a different path would be suitable
However I do not have a strong opinion on this, we can also move to previewConfig if this suits better

Copy link
Member

Choose a reason for hiding this comment

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

If it's in a general section it should at least have a more specific name like messages.prPreviewCreated.
@niiku Any opinion on this?

Copy link
Member

Choose a reason for hiding this comment

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

For me, the general section would make sene - as it is not related to a specific preview. Even though the message content at the end is.

I like the approach from @christiansiegel with a structure for messages and descriptive props, example:

  • messages.previewEnvUpdated
  • messages.previewEnvAlreadyUpToDate
  • messages.previewEnvCreated

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated documentation and configuration path according to your suggestions.

@christiansiegel
Copy link
Member

christiansiegel commented Mar 30, 2022 •
edited

(BTW: I see you're working @baloise. You can become a member of the github organization by adding your handle to the org repo 😉 Example: baloise/org#43)

@christiansiegel
Copy link
Member

FYI: The build currently fails due to a general issue. Will be fixed with #176

christiansiegel
christiansiegel approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member

@christiansiegel christiansiegel 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 me ✔️

niiku
niiku approved these changes Apr 7, 2022
View reviewed changes
Copy link
Member

@niiku niiku left a comment

Choose a reason for hiding this comment

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

lgtm. Thank you very much for your contribution!

@niiku niiku merged commit e859006 into baloise:master Apr 7, 2022
@baopso
Copy link

baopso commented Apr 7, 2022

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@baopso baopso added the released label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joachimprinzbach joachimprinzbach joachimprinzbach left review comments

@niiku niiku niiku approved these changes

@christiansiegel christiansiegel christiansiegel approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@koenigle @christiansiegel @baopso @niiku @joachimprinzbach