-
Notifications
You must be signed in to change notification settings - Fork 179
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
Project Edit Command #1395
Project Edit Command #1395
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
The one change I think is necessary is to give some sort of explanation if the org does not suport projects. Other than that, only minor comments
docs/references/files/fossa-yml.md
Outdated
#### `project.id:` | ||
The project ID defines a unique ID that the FOSSA API will use to reference this project. The project ID can be found in the UI on the project settings page listed as the "Project Locator" underneath the "Project Title" setting. For example, if the "Project Locator" value of `custom+1/foo` is provided in the FOSSA UI, use `foo` for the `project.id`. | ||
The project ID defines a unique ID that the FOSSA API will use to reference this project within your organization. The project ID is a specific portion of the project locator and can be found in the UI on the project `Settings` page listed as the "Project Locator" underneath the "Project Title" setting. For example, if the "Project Locator" value of `custom+1/foo` is provided in the FOSSA UI, use `foo` for the `project.id`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions:
- What happens if the
project.locator
contains a different project id than the value of theproject.id
I supply? - Is it ever necessary to supply both?
- Can I supply just the
project.locator
and have the CLI extract the ID portion for operations that need just the project ID?
If the answer to (3) is yes, I think we should discuss with the team describing the project.id
as deprecated. We'll still need to support it for the foreseeable future, but the UX of "Copy this string from this place in the UI" Is much better than "Copy a specific substring from this place in the UI, good luck hope you get it right!" IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- project.locator only applies for commands
project
andrelease-group
so ifproject.id
is supplied, it just disregards the value fromproject.id
. - Not necessary to supply both as they're used for different commands.
project.locator
->fossa project
&fossa release-group
project.id
->fossa analyze
uses theproject.id
to target specific project revisions - There are complications with extracting the id from the project.locator. When running
fossa analyze
on a new project, users are able to set a customproject.id
. Removingproject.id
means that users lose this functionality, though I am not sure as to how often this feature is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why is it necessary to use a project locator for edit? Based on the description it sounds like a project.id would suffice since a user's API key implies an organization. I'm mainly trying to figure out if we absolutely need these both. They're two values that are very similar so I think it would be easy for a user to use the wrong one.
- The outcome of discussion on (1) aside, I think that this should be spelled out where the values are defined.
- It sounds like what you're saying is that extracting it from a project locator is possible, but it would not be backwards compatible. Is that correct? I think you're right regardless though. Plus, it's a crummy UX to force someone to put their org ID before their project name.
Am I mistaken for thinking that the only difference between the locator and id is that one has the org specifier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most cases, the only difference between locator and id is the org specifier.
Given the case that we have:
project.id
-> example
orgId
-> 1
Usually we get:
project.locator
-> custom+1/example
^ But it isn't always the case that project locators are structured like this (confirmed with core)
In this case we can build up the project locator with just project.id
and we're able to edit this project accordingly.
However, there are some instances where we normalize the project locator. Sometimes we can have:
project.locator
-> git+git/example (or something like this)
In this case there would be no way for us to edit this specific project with just project.id
from the cli.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Projects uploaded through the CLI are structured:
custom+23964/test-package$<version>
So if we only want to limit editing projects to only projects uploaded through the CLI, then we can get rid of project.locator
and just use project.id
.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a discussion out-of-band and these are the notes:
Notes:
- Discussed how you could have a scenario where project info is set by
analyze
but that same block is unusable without adding aproject-locator
key for project edit.- In the current PR
project edit
andrelease-group...
requireproject.locator
butfossa analyze
usesproject.id
- Unreasonable to expect a user to know what the project.locator will be for a yet-to-exist project, so we need to keep project.id
- To properly update projects in core we need to send a full locator
- Analyze gets around this by simply assuming that the locator is something like custom+XXXX/<project.id>
- Solution(s):
- When project edit sees only project.id , do the same thing as analyze to generate a locator.
- Also: either forbid both project.id and project.locator from being set OR prefer project.locator to project.id.
- For consistency, analyze needs to be set up to pass project.locator through I think. No matter the reason, all commands should select the same project given the same configuration.
- In the current PR
- Because organizations can require a
--policy
on project creation we can't begin deprecating thefossa analyze
project flags until we have afossa project create
command. - From after our talk: I think that project edit should also accept policy-id if it doesn't. See the analyze flags.
There were a lot of implementation changes due to the push back from Core. The changes made don't rely on the api/cli/project endpoint I initially created in Core. Instead, we are using existing endpoints to achieve editing project metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two big questions.
First, is the change from team
to teams
in .fossa.yml a breaking change.
Second, should you be able to remove teams from a project using this command. Right now you can only add teams.
A few small nits as well, but let's focus on those right now
deriving (Eq, Ord, Show) | ||
|
||
instance ToJSON TeamProjectAction where | ||
toJSON Add = "add" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the teams addition idempotent by changing this from add
to replace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested this and replace
actually replaces all the projects for a given team. This means that if used replace
, the only the project that we are using through fossa project edit
will be present in that team. It overwrites all the existing team projects.
I think the functionality that we're looking for is the inverse of what was previously described. We want to replace all the teams that are associated with a project with the provided list of teams . Unfortunately, this isn't a functionality that is supported through the existing Core endpoints.
I think the best thing to do here is the provide a flag through fossa project edit
such as --remove-from-teams
or --team-action
(Add, Remove), this will allow removing the project from all the listed teams provided through the command. That way we have some way remove projects from teams. We would just need to add remove
as a team action.
I've updated the edit.md to let users know that currently fossa project edit
only allows adding projects to teams.
Going to create a ticket to add this flag!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for looking into the team/teams thing, and it's too bad that we can't update the teams idempotently.
I think you'll still need to checkin with Chris to make sure you've addressed the changes that they requested.
Chris initially took over the pr review when Scott OOO. I've talked to Chris and we decided that we would leave the final review to Scott as he was assigned initially.
Overview
Acceptance criteria
Testing plan
Added unit tests
Project Locator Manual Testing
example .fossa.yml:
fossa project edit --project-locator custom+1/example --title example-title --project-url github.com/fossas/fossa-cli --jira-project-key example-jira-key --link fossa.com --team example-team-1 --team example-team-2 --policy example-policy --project-label example-label-1 --project-label example-label-2
Project ID Manual Testing
example .fossa.yml:
fossa project edit --project-id example --title example-title --project-url github.com/fossas/fossa-cli --jira-project-key example-jira-key --link fossa.com --team example-team-1 --team example-team-2 --policy example-policy --project-label example-label-1 --project-label example-label-2
Risks
Metrics
References
Checklist
docs/
.docs/README.ms
and gave consideration to how discoverable or not my documentation is.Changelog.md
. If this PR did not mark a release, I added my changes into an# Unreleased
section at the top..fossa.yml
orfossa-deps.{json.yml}
, I updateddocs/references/files/*.schema.json
AND I have updated example files used byfossa init
command. 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
).docs/references/subcommands/<subcommand>.md
.