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: allow to pass mfa for git sha, git author and app version to lifecycle annotator #163

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yordis
Copy link

@yordis yordis commented Aug 25, 2022

Change description

What problem does this solve?

PromEx.LifecycleAnnotator forces us to use GIT_SHA, GIT_AUTHOR and vsn application spec to configure the annotation.

We would like to have the ability, as we do with PromEx.Plugins.Application, to pass an MFA configuration for all those values so we can customize it to our needs.

Issue number: (if applicable)

Example usage

Additional details and screenshots

Checklist

  • I have added unit tests to cover my changes.
  • I have added documentation to cover my changes.
  • My changes have passed unit tests and tested E2E in an example project.

@yordis
Copy link
Author

yordis commented Aug 25, 2022

@akoutmos do you mind giving me your perspective here?

I am trying to allow to pass some MFA config because we do not use the current env variable names for the git author or git sha, and we would like to use an environment name for the app release version.

I am not sure what the configuration should look like.

For example:

config :app_name, MyApp.PromEx,
  lifecycle_annotator: []

Or

config :app_name, MyApp.PromEx.LifecycleAnnotator,
  [
    # ...
  ]

I am liking the latter since it is easier to implement and maintain.

@akoutmos
Copy link
Owner

I would actually prefer the first version over the second as the second one leaks the internal implementation of the library to the user. What GenServers are started under the PromEx supervision tree is an implementation detail and could change in the future.

Let's move forward with the first version, and soft deprecate the :annotate_app_lifecycle config in favor of :lifecycle_annotator (https://github.com/akoutmos/prom_ex/blob/master/lib/prom_ex/config.ex#L121) with the functionality you propose for having MFAs as opposed to env vars.

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