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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance/enable tag policies api implicitly #2574

Merged
merged 2 commits into from
Jul 14, 2023

Conversation

tim775
Copy link
Member

@tim775 tim775 commented Jul 13, 2023

No description provided.

@tim775 tim775 requested a review from hugorut July 13, 2023 21:52
@tim775 tim775 self-assigned this Jul 13, 2023
Copy link
Contributor

@hugorut hugorut left a comment

Choose a reason for hiding this comment

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

Not sure I follow exactly why we're making this change? Requiring the URL to be set to toggle ctx.Config.TagPoliciesEnabled seems like we'll duplicate the tag policy checks. As we later use and empty check against the URL in comment to check the policies from the CLI. Should we also change these to check against ctx.Config.TagPoliciesEnabled? We'll also need to explicitly add the policies URL in the runner env to make sure this works.

@tim775
Copy link
Member Author

tim775 commented Jul 14, 2023

Requiring the URL to be set to toggle ctx.Config.TagPoliciesEnabled seems like we'll duplicate the tag policy checks.

I don't think that's quite right. This does two things 1) if TagsAPIEnabled from the cloud settings is true, it makes sure the API URL is set 2) if the API URL is set, it sets TagPoliciesEnabled=true.

I agree it's a little convoluted. Maybe it would be simpler to remove the TagPoliciesEnabled flag and just check for TagPolicyAPIEndpoint != ""?

We'll also need to explicitly add the policies URL in the runner env to make sure this works.

The CLI in runner shouldn't ever use the tag policies API since the runner does the check directly. Runner sets CLOUD_UPLOAD_ENABLED=false to make sure that's the case.

Copy link
Contributor

@hugorut hugorut left a comment

Choose a reason for hiding this comment

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

makes sense

@tim775 tim775 merged commit 1798801 into master Jul 14, 2023
9 of 10 checks passed
@tim775 tim775 deleted the enhance/enable_tag_policies_api_implicitly branch July 14, 2023 13:11
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.

None yet

2 participants