Track git tags #517
Track git tags #517
Conversation
✅ Deploy Preview for opal-docs canceled.
|
philipclaesson
force-pushed
the
master
branch
3 times, most recently
from
26f51fa
to
8d46775
Compare
|
This is great 💜, I think we can review it early /.mid next week. @philipclaesson please note that tests and formatting checks are already alerting issues we'll have to fix before this can be merged. |
philipclaesson
force-pushed
the
master
branch
from
8d46775
to
2794154
Compare
|
Thanks @orweis! Formatting fixed now which made tests pass. I might add one or two test cases next week. |
| @@ -49,7 +51,16 @@ def __init__( | |||
| ssh_key=self._ssh_key, | |||
| clone_timeout=request_timeout, | |||
| ) | |||
|
|
|||
| if branch_name is None and tag_name is None: | |||
| logger.exception("Must provide either branch_name or tag_name") | |||
There was a problem hiding this comment.
Since we are not within in a except: clause it's more correct to use logger.error
See: https://docs.python.org/3/library/logging.html#logging.Logger.exception
Also maybe best to include the actual ENV-VAR name- as the average user will struggle with translating
There was a problem hiding this comment.
Alternatively you can raise the exception here, and just log it in a higher except clause- which would be nicer.
There was a problem hiding this comment.
I see, thanks! Would that mean wrapping this in a try/except?
There was a problem hiding this comment.
That sounds like a good option, yes
| self._tracker = BranchTracker( | ||
| repo=repo, branch_name=self._branch_name, ssh_key=self._ssh_key | ||
| ) | ||
| if self._tag_name is not None: |
| @@ -99,7 +99,8 @@ class OpalServerConfig(Confi): | |||
| False, | |||
| "Set if OPAL server should use a fixed clone path (and reuse if it already exists) instead of randomizing its suffix on each run", | |||
| ) | |||
| POLICY_REPO_MAIN_BRANCH = confi.str("POLICY_REPO_MAIN_BRANCH", "master") | |||
| POLICY_REPO_MAIN_BRANCH = confi.str("POLICY_REPO_MAIN_BRANCH", None) | |||
| POLICY_REPO_TAG = confi.str("POLICY_REPO_TAG", None) | |||
There was a problem hiding this comment.
This is great, but notice that this only works with OPAL working with a single repo: branch/tag - and won't enable this feature for the OPAL scopes option.
If you're feeling up to it, it might be worth adding there as well
There was a problem hiding this comment.
Right - I'll add it there as well. I've never used OPAL scopes myself so I'll need to read up a bit on how to test it.
|
Hi @philipclaesson @orweis - Where those that PR stand? What changes are still pending? Adding it to scopes as well? For now moving this to draft |
This change was discussed on slack and @orweis said a contribution adding tracking of git tags could be considered.
Changes proposed
Why tracking git tags instead of branch
In a way this could be considered "against" how git is supposed to be used - some say tags should be immutable. However, mutable tags are defacto sometimes used, the most clear example being using a "latest" tag or a "v1-latest" tag.
Using tags this way could let you use a policy repo with a single base branch, while still giving control of what is rolled out in each env. Consider the traditional dev/prod setup:
devinstance of OPAL is tracking the base branch (ex.master)prodinstance of OPAL is tracking aprodtag which can be moved at will once the changes in dev/master is tested properly.This current way of doing this is to have multiple long lived branches in the same repo - this can be cumbersome and lead to a bunch of manual merging of branches.
Changes
POLICY_REPO_TAGand cli arg--policy-repo-tag, defaulting to None. When used, OPAL will track the tag.POLICY_REPO_TAGandPOLICY_REPO_MAIN_BRANCHThe two variables are obviously dependent on eachother. My idea was to not break default behaviour.
masterbranchCheck List (Check all the applicable boxes)
Note to reviewers