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

Feat: Refactor for Project Role assignments for Team #820

Merged
merged 3 commits into from
Apr 7, 2024

Conversation

TomerHeber
Copy link
Collaborator

@TomerHeber TomerHeber commented Apr 6, 2024

Solution

Part of #817. Will create another PR for refactoring the API.

  1. Modifed the API code.
  2. Updated unit tests.
  3. Modified the resource code to use the new API.
  4. Updated acceptance tests.

@github-actions github-actions bot added ready to merge PR approved - can be merged once the PR owner is ready and removed pending final review labels Apr 7, 2024
@razbensimon
Copy link
Contributor

razbensimon commented Apr 7, 2024

@TomerHeber @sagydr Do we need a non-patch upgrade here?
i wonder how a new tf plan will look like after apgrading

@TomerHeber
Copy link
Collaborator Author

@TomerHeber @sagydr Do we need a non-patch upgrade here? i wonder how a new tf plan will look like after apgrading

@razbensimon - the plan should remain the same. Is there a way to test it? I can do a new major version if it makes sense. WDYT?

@razbensimon
Copy link
Contributor

razbensimon commented Apr 7, 2024

if we can do it with a patch it is awesome, I just bring it up for you to test it.
meaning "upgrading" after having an existing project assignment in the state.
then try to:

  1. plan/apply with no changes
  2. delete assignment
  3. change assignment
  4. add assignment

EDIT:
mostly I afraid of the ProjectRole & Role renames. how it will effect the state/plans?

Comment on lines +64 to +65
TeamProjectAssignmentCreateOrUpdate(payload *TeamProjectAssignmentPayload) (*TeamProjectAssignment, error)
TeamProjectAssignmentDelete(projectId string, teamId string) error
Copy link
Contributor

@razbensimon razbensimon Apr 7, 2024

Choose a reason for hiding this comment

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

I think "api_client" should have only 3 methods, that serve all scopes for team (and additional 3 methods for users).
the methods are the upsert / remove / get assignments - corresponding to the API.

So each method will get the scopeId as an argument (aka environmentId, projectId, or organizationId. They are optional and only one of them should exist).
That's because all of those are the same API and routes.

meaning TeamProjectAssignmentCreateOrUpdate and TeamProjectAssignmentDelete should be removed. And we should not "drag" / keep the existence of the old deprecated APIs names.

WDYS?

Copy link
Contributor

Choose a reason for hiding this comment

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

the only scopes we haven't refactored yet - should preserve their methods here. Until we will refactor them too.
that's include:

  1. org level user assignments
  2. project level user assignments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will be done in a follow-up PR.

@TomerHeber
Copy link
Collaborator Author

if we can do it with a patch it is awesome, I just bring it up for you to test it. meaning "upgrading" after having an existing project assignment in the state. then try to:

  1. plan/apply with no changes
  2. delete assignment
  3. change assignment
  4. add assignment

EDIT: mostly I afraid of the ProjectRole & Role renames. how it will effect the state/plans?

As discussed on Slack. Only concern is if someone had mistakenly applied multiple assignments on the same project and team. (Not supported in the UI).

@TomerHeber TomerHeber merged commit f5f06ea into main Apr 7, 2024
9 checks passed
@TomerHeber TomerHeber deleted the feat-refactor-project-team-#817 branch April 7, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-client feature provider ready to merge PR approved - can be merged once the PR owner is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants