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

Track git tags #517

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

philipclaesson
Copy link
Contributor

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:

  • the dev instance of OPAL is tracking the base branch (ex. master)
  • the prodinstance of OPAL is tracking a prod tag 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

  • Add env variable POLICY_REPO_TAG and cli arg --policy-repo-tag, defaulting to None. When used, OPAL will track the tag.

POLICY_REPO_TAG and POLICY_REPO_MAIN_BRANCH

The two variables are obviously dependent on eachother. My idea was to not break default behaviour.

POLICY_REPO_TAG POLICY_REPO_MAIN_BRANCH Behaviour
Empty Empty OPAL tracks the master branch
Empty Not Empty OPAL tracks the given branch
Not Empty Empty OPAL tracks the given tag
Not Empty Not Empty OPAL fails (cannot decide what to track)

Check List (Check all the applicable boxes)

  • I sign off on contributing this submission to open-source
  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Note to reviewers

  • Some of tests are a bit flaky on my local machine - but all relevant and new tests should pass
  • I have updated documentation where applicable (I think) - let me know if I've missed anywhere
  • This PR only affects polling a git repo - not via webhooks. I am not sure whether you think webhooks + tag is a strong use case. If you want, I can look into that use case as well.

Copy link

netlify bot commented Nov 17, 2023

Deploy Preview for opal-docs canceled.

Name Link
🔨 Latest commit 2794154
🔍 Latest deploy log https://app.netlify.com/sites/opal-docs/deploys/655797783f2c7b0007a4a1dc

@orweis
Copy link
Contributor

orweis commented Nov 17, 2023

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
Copy link
Contributor Author

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@orweis orweis Nov 19, 2023

Choose a reason for hiding this comment

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

Alternatively you can raise the exception here, and just log it in a higher except clause- which would be nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks! Would that mean wrapping this in a try/except?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a good option, yes

Copy link
Contributor

@orweis orweis left a comment

Choose a reason for hiding this comment

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

Really great stuff- a few slight tweaks requested (As standalone comments) and some optional suggestions for improvement

self._tracker = BranchTracker(
repo=repo, branch_name=self._branch_name, ssh_key=self._ssh_key
)
if self._tag_name is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@roekatz
Copy link
Collaborator

roekatz commented Apr 30, 2024

Hi @philipclaesson @orweis - Where those that PR stand? What changes are still pending? Adding it to scopes as well?
LMK if and how I can help with the remaining work.

For now moving this to draft

@roekatz roekatz marked this pull request as draft April 30, 2024 13:08
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.

3 participants