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-9189: Remove PLAN_TAG_REVISION_STATE from planTag #126

Merged
merged 1 commit into from
Sep 15, 2023
Merged

Conversation

manelcecs
Copy link
Contributor

@manelcecs manelcecs commented Sep 14, 2023

Remove duplicated plan revision state codecs from plan and planTag models

@manelcecs manelcecs requested a review from a team as a code owner September 14, 2023 10:04
@manelcecs manelcecs added the ready for review All comments have been addressed, and the Pull Request is ready for review label Sep 14, 2023
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.

One minor thing inline. I will fix it myself. Regarding what you wrote in the first comment:

Remove duplicated codecs from plan and planTag models in hpc-api-core.

Once this is merge, we need to update both HPC_service and HPC-api

  • We are in hpc-api-core repo, so no need to be specific about this :)
  • We don't need to update hpc_service and hpc-api to include the latest version because they can keep functioning with how things were. The only reason why we would potentially need to update is if any of them was specifically importing PLAN_TAG_REVISION_STATE, which we now removed. Therefore, I would say these changes can wait and need not be released immediately, but we can bundle them with some other change which affects hpc_service/hpc-api directly and release at that time.

@@ -4,6 +4,7 @@ import { brandedType } from '../../util/io-ts';
import type { Brand } from '../../util/types';
import { defineIDModel } from '../util/id-model';
import { PLAN_ID } from './plan';
import { PLAN_REVISION_STATE } from './plan';
Copy link
Contributor

Choose a reason for hiding this comment

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

You import from same file on two different lines, you should combine them into:

import { PLAN_ID, PLAN_REVISION_STATE } from './plan';

@Pl217 Pl217 added ready for merge Review and testing is complete. It is ready for merging as soon as CI has finished. and removed ready for review All comments have been addressed, and the Pull Request is ready for review labels Sep 15, 2023
@Pl217 Pl217 self-assigned this Sep 15, 2023
@Pl217 Pl217 merged commit 65e3ff3 into develop Sep 15, 2023
1 check passed
@Pl217 Pl217 deleted the HPC-9189 branch September 15, 2023 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for merge Review and testing is complete. It is ready for merging as soon as CI has finished.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants