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

HPC-9944: Add v4 models to Role and WorkflowRole tables #212

Closed
wants to merge 2 commits into from

Conversation

Delgee
Copy link
Contributor

@Delgee Delgee commented Jan 20, 2025

No description provided.

@Delgee Delgee added the ready for review All comments have been addressed, and the Pull Request is ready for review label Jan 20, 2025
@Delgee Delgee requested a review from a team as a code owner January 20, 2025 14:47
Copy link
Contributor

@manelcecs manelcecs left a comment

Choose a reason for hiding this comment

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

Please review the comment! Other than that is looking fine

export type WorkflowRoleId = Brand<
number,
{ readonly s: unique symbol },
'workflowRole.id'
>;

export const WORKFLOW_ROLE_ID = brandedType<number, WorkflowRoleId>(t.number);

// export const PERMITTED_ACTION_IDS = t.union([
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this unused code

Copy link
Contributor

@Pl217 Pl217 left a comment

Choose a reason for hiding this comment

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

The reason why v4 models don't have these tables is that they are considered legacy and should not be used in any new code. Since you need to write temporary admin command, which must fix corrupt legacy data, I think you can use legacy models for that purpose.

In related hpc_service PR, if you can write all code using v4 models and only use legacy models for legacy tables, that would be best. But, if combining models isn't possible in some place, it's fine to use legacy models there too, since it is a temporary thing, fixing legacy stuff.

@Pl217 Pl217 assigned Delgee and unassigned manelcecs Jan 21, 2025
@Pl217 Pl217 added needs major changes There are review or issue comments to address that will result in big changes and removed ready for review All comments have been addressed, and the Pull Request is ready for review labels Jan 21, 2025
@Delgee Delgee closed this Jan 29, 2025
@Delgee
Copy link
Contributor Author

Delgee commented Jan 29, 2025

No longer needed.

@Pl217 Pl217 deleted the HPC-9944 branch January 29, 2025 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs major changes There are review or issue comments to address that will result in big changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants