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

[SNC-392] compile support for policy test command #973

Merged
merged 4 commits into from
Jul 31, 2023

Conversation

davidmdm
Copy link
Contributor

@davidmdm davidmdm commented Jul 27, 2023

Checklist

=========

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked for similar issues and haven't found anything relevant.
  • This is not a security issue (which should be reported here: https://circleci.com/security/)
  • I have read Contribution Guidelines.

Internal Checklist

  • I am requesting a review from my own team as well as the owning team
  • I have a plan in place for the monitoring of the changes that I am making (this can include new monitors, logs to be aware of, etc...)

Changes

=======

  • Updated the CircleCI-Public/circle-policy-agent
  • Modified the circleci policy test command by adding the policy-agent compile function
    • calls the orbs-service API via graphql to compile test input
  • Fixes a bug in the policy API client that was causing failures across tests
  • adds the compily policy and test to validate the compilation is working in e2e fashion

Rationale

=========

To provide a better testing experience of CircleCI Config Policies, we needed to update the policy-agent that understood the compile and pipeline_parameters directive, and provide a compilation function for it to use.

@davidmdm davidmdm requested a review from a team July 27, 2023 14:25
@davidmdm davidmdm requested a review from a team as a code owner July 27, 2023 14:25
@davidmdm davidmdm changed the title Snc 392 compile support for policy test command [SNC-392] compile support for policy test command Jul 27, 2023
@davidmdm davidmdm changed the base branch from develop to main July 27, 2023 15:09
Copy link

@jbialy jbialy left a comment

Choose a reason for hiding this comment

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

From a high level it lgtm!

@@ -490,6 +494,44 @@ This group of commands allows the management of polices to be verified against b
runnerOpts := tester.RunnerOptions{
Path: args[0],
Include: include,
Compile: func(data []byte, pipelineValues map[string]any) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

feels like a good candidate for a separate function outside of command definition

@@ -1330,13 +1331,31 @@ func TestTestRunner(t *testing.T) {
assert.Contains(t, s, "<?xml")
},
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Test suggestion:
compile: true, but owner-id isn't provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the current test case

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is if compilation is required, but owner-id isn't provided, it should fail in some way.
But the expectation is pass in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Owner-ID is only required if you need to pull a private orb

@davidmdm davidmdm merged commit 0fd0133 into main Jul 31, 2023
@davidmdm davidmdm deleted the SNC-392-compile-support-for-policy-test-command branch July 31, 2023 13:25
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.

5 participants