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

Ane 873 - support --policy-id #1203

Merged
merged 14 commits into from
May 31, 2023
Merged

Ane 873 - support --policy-id #1203

merged 14 commits into from
May 31, 2023

Conversation

csasarak
Copy link
Contributor

@csasarak csasarak commented May 24, 2023

Overview

This PR makes it so that the CLI can accept a new policy id argument as part of require policy work.

This will likely merge after #1202 because removing wiggins will simplify this PR.

Acceptance criteria

  • Should be able to set a policy id with --policy-id <num> or using the policyId: <id> directive in .fossa.yml.
  • Only one of a policy name or policy id can be set. This rule applies to mixed CLI and config file arguments also.

Testing plan

  1. Set the require policy flag for an organization.
  2. Scan a project, making sure to add --project <name> with a name that has never been used before. A previously deleted project does not behave as a new project on this endpoint. ANE-462 is meant to address this. It should fail with an error about a missing policy.
  3. Run fossa analyze with the --policy-id <number> with the id of a policy. You can get this by navigating to the policy's page in the web UI and looking at the URL. It should succeed.

Run fossa analyze --policy name --policy-id 1, it should fail. Likewise if you put both or one of the directives in .fossa.yml.

Risks

References

ANE-873
Core PR to support policy-id: https://github.com/fossas/FOSSA/pull/10171

Checklist

  • I added tests for this PR's change (or explained in the PR description why tests don't make sense).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • If this change is externally visible, I updated Changelog.md. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • If I made changes to .fossa.yml or fossa-deps.{json.yml}, I updated docs/references/files/*.schema.json. You may also need to update these if you have added/removed new dependency type (e.g. pip) or analysis target type (e.g. poetry).

@csasarak csasarak marked this pull request as ready for review May 24, 2023 21:27
@csasarak csasarak requested a review from a team as a code owner May 24, 2023 21:27
@csasarak csasarak requested a review from jssblck May 24, 2023 21:27
@csasarak csasarak changed the title Ane 873 require policy Ane 873 - support --policy-id May 24, 2023
@jssblck jssblck mentioned this pull request May 24, 2023
4 tasks
Comment on lines 116 to 120
#### `project.policy:`
The name of the policy in your FOSSA organization to associate this project with.
The name of the policy in your FOSSA organization to associate this project with. Mutually excludes `project.policyId`.

#### `project.policyId:`
The id of the policy in your FOSSA organization to associate this project with. Mutually excludes `project.policy`.
Copy link
Member

Choose a reason for hiding this comment

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

Please document the exact semantics, since these are both marked "mutually excludes".

Reading the code, I believe that if both are provided, policy takes precedence.
If this is correct, please clearly document everywhere we document these flags/settings.

I recommend using wording similar to:

#### `project.policy:`
The name of the policy in your FOSSA organization with which to associate this project. 

If this option and `project.policyId` are both provided, 
this option takes precedence and causes `project.policyId` to be ignored.

#### `project.policyId:`
The id of the policy in your FOSSA organization with which to associate this project. 

If this option and `project.policy` are both provided, 
`project.policy` takes precedence and causes this option to be ignored.

Copy link
Member

@jssblck jssblck May 24, 2023

Choose a reason for hiding this comment

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

[optional] I think it'd also be helpful to tell users how to get the ID of a given policy.

If we don't have existing Core-side docs to point them to for this, please consider writing a quick doc in docs.fossa.com to link to.

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've added a note here about how the CLI variants of these values take precedence over config file ones. From our other discussion here I think that mutually excludes is the right language otherwise.

I will look into making a doc tomorrow on where to get a policy id, right now the main way I know of to get them is from the URL of the policy page.

src/App/Fossa/Config/Common.hs Show resolved Hide resolved
src/App/Fossa/Config/ConfigFile.hs Show resolved Hide resolved
@csasarak csasarak requested a review from jssblck May 30, 2023 23:40
@csasarak csasarak merged commit b09ebd6 into master May 31, 2023
17 checks passed
@csasarak csasarak deleted the ANE-873-require-policy branch May 31, 2023 17:13
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