-
Notifications
You must be signed in to change notification settings - Fork 7
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
Esas/replace deprecated endpoints v3 2 prod 13356 #843
Conversation
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
scenarios.map((scenario) => ({ | ||
id: scenario.id, | ||
runId: scenario.lastRun?.scenarioRunId, | ||
csmSimulationRun: scenario.lastRun?.csmSimulationRun, |
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.
If there is no equivalent for csmSimulationRun
for runners, then we need to remove the related options of the PowerBI dynamic filters from the code (webapp, ui (not sure) & azure repositories) and the documentation.
We've been offering two dynamic filters options csmSimulationRun
and visibleScenariosCsmSimulationRunsIds
(c.f. here) for the PowerBI dashboards, these options should probably be removed when migrating to runners.
This is a breaking change that should be documented in the commit message (this way it will be automatically included in the generated changelog).
Also, we may be using these options in our brewery workspaces: we have to update the workspace description files in the brewery_sample_solution repository to use another dynamic filter
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.
As I understood, the run property replaces csmSimulationRun
from back-end point of view, can it be done in powerBi filter as well or it's trickier than that ?
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.
Indeed, it seems that the Run object (replacing the ScenarioRun) still has a property csmSimulationRun
, so we should try to keep supporting the csmSimulationRun
option.
In the last line of the highlighted code, we're removing the csmSimulationRun
property, is there a reason preventing us to keep it?
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 deleted it because runner object has only lastRunId
property which is equal to csmSimulationRun
and I didn't think about powerBi 🤦♀️ if we keep both properties in runner object in Redux, will it help to keep filters or we'd better update filters to use lastRunId
key ? 🤔
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.
Good question, we can ask the PO 😄
Maybe keeping the two filters could help to decrease the cost of version upgrade. Yet we should probably define one of the two filters as deprecated, so we can remove it in the future
src/services/AppInsights.js
Outdated
|
||
trackRunnerRunDuration(runDuration) { | ||
if (this.enabled) { | ||
this.appInsights.trackMetric({ name: 'RunDurationValue', average: runDuration }); | ||
} | ||
} |
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.
Do we need a new function ? If trackScenarioRunDuration
is no longer used, maybe we can rename it ? Or even simpler, maybe we could keep the names of the previous trackScenario*
functions ?
src/services/config/ApiConstants.js
Outdated
export const RUNNER_RUN_STATE = { | ||
CREATED: 'Created', | ||
RUNNING: 'Running', | ||
SUCCESSFUL: 'Successful', | ||
FAILED: 'Failed', | ||
UNKNOWN: 'Unknown', | ||
}; |
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.
Should we also remove the previous state values? (SCENARIO_RUN_STATE
)
export const SCENARIO_STATUS_POLLING_DELAY = 10000; | ||
export const RUNNER_STATUS_POLLING_DELAY = 10000; |
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.
Should we remove SCENARIO_STATUS_POLLING_DELAY
?
const response = yield call(Api.RunnerRuns.getRunStatus, organizationId, workspaceId, runnerId, lastRunId); | ||
yield put({ type: RUNNER_ACTIONS_KEY.UPDATE_RUNNER, data: { runnerState: response.data.state, runnerId } }); | ||
} | ||
export function* listAllRunners(organizationId, workspaceId) { |
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.
IIUC, this saga only keeps the "simulation runners" (excluding the "ETL runners"), shouldn't we rename it to something like getSimulationRunners
instead ?
const solutionParameters = yield select(getSolutionParameters); | ||
const { data } = yield call(Api.Runners.listRunners, organizationId, workspaceId); | ||
const simulationRunTemplatesIds = runTemplates | ||
.filter((rt) => rt.tags?.some((tag) => !['datasource', 'subdatasource'].includes(tag))) |
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.
Not mandatory, but the filter could reuse the functions isDataSource
and isSubDataSource
from file src/utils/SolutionsUtils.js
const runTemplates = yield select(getSolutionRunTemplates); | ||
const runnersPermissionsMapping = yield select(getRunnersPermissionsMapping); | ||
const solutionParameters = yield select(getSolutionParameters); | ||
const { data } = yield call(Api.Runners.listRunners, organizationId, workspaceId); |
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.
As discussed, this endpoint uses pagination, we need to force a very large page size to make sure we receive all runners 🙂
0e5b54b
to
295210f
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
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 feedbacks on the cypress tests refactoring 🙂
@@ -71,23 +69,11 @@ function writeInScenarioSelectorInput(searchStr) { | |||
// the edit button after switching to the selected scenario; this option can be useful when the connected user does | |||
// not have the edit permissions on the selected scenario (i.e. when the edit button is not visible) | |||
function selectScenario(scenarioName, scenarioId, options) { | |||
const getScenarioAlias = api.interceptGetScenario(scenarioId); | |||
// const getScenarioAlias = api.interceptGetRunner(scenarioId); |
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.
Commented code to remove 🙂
|
||
// return api.waitAlias(getScenariosAlias).then((interception) => { | ||
// const nameGet = interception.response.body.find((obj) => obj.id === scenarioCreated.id).name; | ||
// expect(nameGet).to.equal(scenarioCreated.name); | ||
// | ||
// return { | ||
// scenarioCreatedId: scenarioCreated.id, | ||
// scenarioCreatedName: scenarioCreated.name, | ||
// scenarioCreatedOwnerName: scenarioCreated.ownerName, | ||
// scenarioCreatedCreationDate: scenarioCreated.creationDate, | ||
// scenarioCreatedRunTemplateName: runTemplate, | ||
// scenarioCratedDatasetOrMasterName: datasetOrMasterName, | ||
// }; | ||
// }); | ||
} | ||
|
||
function validateScenario(scenarioId) { | ||
const validateScenarioAlias = api.interceptUpdateScenario({ scenarioId }); | ||
const getScenarioAlias = api.interceptGetScenario(scenarioId); | ||
const validateScenarioAlias = api.interceptUpdateSimulationRunner({ scenarioId }); | ||
// const getScenarioAlias = api.interceptGetRunner(scenarioId); | ||
|
||
getScenarioValidateButton().click(); | ||
Scenarios.getScenarioValidationStatusLoadingSpinner().should('be.visible'); | ||
// Scenarios.getScenarioValidationStatusLoadingSpinner().should('be.visible'); | ||
|
||
api.waitAlias(validateScenarioAlias); | ||
api.waitAlias(getScenarioAlias); | ||
// api.waitAlias(getScenarioAlias); | ||
} | ||
function rejectScenario(scenarioId) { | ||
const rejectScenarioAlias = api.interceptUpdateScenario({ scenarioId }); | ||
const getScenarioAlias = api.interceptGetScenario(scenarioId); | ||
const rejectScenarioAlias = api.interceptUpdateSimulationRunner({ scenarioId }); | ||
// const getScenarioAlias = api.interceptGetRunner(scenarioId); | ||
|
||
getScenarioRejectButton().click(); | ||
Scenarios.getScenarioValidationStatusLoadingSpinner().should('be.visible'); | ||
// Scenarios.getScenarioValidationStatusLoadingSpinner().should('be.visible'); | ||
|
||
api.waitAlias(rejectScenarioAlias); | ||
api.waitAlias(getScenarioAlias); | ||
// api.waitAlias(getScenarioAlias); | ||
} | ||
function resetScenarioValidationStatus(scenarioId) { | ||
const resetScenarioAlias = api.interceptUpdateScenario({ scenarioId }); | ||
const getScenarioAlias = api.interceptGetScenario(scenarioId); | ||
const resetScenarioAlias = api.interceptUpdateSimulationRunner({ scenarioId }); | ||
// const getScenarioAlias = api.interceptGetRunner(scenarioId); | ||
|
||
getScenarioValidationStatusChipDeleteIcon().click(); | ||
Scenarios.getScenarioValidationStatusLoadingSpinner().should('be.visible'); | ||
// Scenarios.getScenarioValidationStatusLoadingSpinner().should('be.visible'); | ||
|
||
api.waitAlias(resetScenarioAlias); | ||
api.waitAlias(getScenarioAlias); | ||
// api.waitAlias(getScenarioAlias); |
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.
More commented code to remove 😀
RUNNER_DEFAULT_SECURITY: URL_ROOT + '/.*/runners/((r|R)-[\\w]+)/security/default', | ||
START_RUNNER: URL_ROOT + '/.*/runners/((r|R)-[\\w]+)/start', // Endpoint to start the runner | ||
RUNNER_STATE: URL_ROOT + '/.*/runs/(run-[\\w]+)/status', | ||
SIMULATION_RUNNER_DEFAULT_SECURITY: URL_ROOT + '/.*/runners/((r|R)-[\\w]+)/security/default', |
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.
SIMULATION_RUNNER_DEFAULT_SECURITY
could be renamed to RUNNER_DEFAULT_SECURITY
, because the endpoint is not specific to scenarios (what we call "simulation runners").
Same thing for API_REGEX.SIMULATION_RUNNER_DEFAULT_SECURITY
🙂
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 created a new constant because the regex stored in RUNNER_DEFAULT_SECURITY
doesn't match needed endpoint:
'/.*/runners/((**d|D**)-[\\w]+)/security/default'
It matches dataset security for some reasons, so I created a new one 🙂
cypress/commons/services/stubbing.js
Outdated
@@ -232,6 +233,7 @@ class Stubbing { | |||
|
|||
patchScenarioACLSecurity = (scenarioId, newACLSecurityItem) => { | |||
const scenario = this.getScenarioById(scenarioId); | |||
console.log(scenario); |
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.
Remaining console log 😉
cypress/commons/utils/apiUtils.js
Outdated
const lastRun = stub.getScenarioRunById(scenarioRunId); | ||
const stubbedStartTime = stub.getScenarioRunOptions().startTime; | ||
if (stubbedStartTime !== undefined) lastRun.startTime = stubbedStartTime; | ||
// add change os state in stubbing like lines 395-402 |
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.
Is the comment still relevant?
cypress/commons/utils/routeUtils.js
Outdated
@@ -74,7 +74,7 @@ const browse = (options) => { | |||
const go = (direction, options) => { | |||
const aliases = []; | |||
if (options?.workspaceId) aliases.push(...api.interceptSelectWorkspaceQueries()); | |||
if (options?.scenarioId) aliases.push(api.interceptGetScenario(options?.scenarioId)); | |||
// if (options?.scenarioId) aliases.push(api.interceptGetRunner(options?.scenarioId)); |
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.
Commented line to remove :)
@@ -28,6 +28,7 @@ describe('scenario parameters inputs validation', () => { | |||
GET_WORKSPACES: true, | |||
GET_ORGANIZATION: true, | |||
UPDATE_SCENARIO: true, | |||
// PERMISSIONS_MAPPING: true, |
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.
Is there a reason to remove the stubbing of the permissions endpoint in this test? (and to enable it in the test ScenarioMetadataParameters?)
Is it related to missing information in the mapping sent by the Cosmo API?
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.
It is related to hours when I launched the test 🤣 I launched it when server wasn't running yet, so I added the missing stubbing endpoint to be fully independent from the API, then I disabled it to check that it changes nothing. I will remove it to keep our tests as it was 🙂
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.
Ok, we can keep it then if it helps running our stubbed tests when the API is off 🙂
return isCurrentScenarioRunning ? ( | ||
<Grid item> | ||
<Stack direction="row" spacing={1} sx={{ mr: 2 }}> | ||
<CircularProgress color="inherit" size={16} data-cy="running-state-spinner" /> | ||
<Typography variant="body1" data-cy="running-state-label"> | ||
{currentScenarioState === SCENARIO_RUN_STATE.RUNNING | ||
{currentScenarioState === RUNNER_RUN_STATE.RUNNING |
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.
This condition can now be replaced by isCurrentScenarioRunning
.
I think we can even remove the condition and keep only the "running" label
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.
Nice catch, thanks ! 🙂
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.
LGTM 👍
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
61df628
to
1741bb7
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
1741bb7
to
d9c88ab
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
d9c88ab
to
84a4250
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
84a4250
to
d5f2dde
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
d5f2dde
to
0c0252f
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
1 similar comment
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
6129c59
to
be610c6
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
2732132
to
fd25fb2
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
fd25fb2
to
b5c964f
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
1 similar comment
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
99db836
to
676e900
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
676e900
to
28e403d
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
BREAKING CHANGE: replacing scenarios endpoints by runners needs changes in run templates, CSM_SCENARIO_ID and CSM_SCENARIO_RUN_ID env vars must be replaced by CSM_RUNNER_ID and CSM_RUN_ID
… doesn't exist anymore
…wingraph_dataset sourceType
28e403d
to
883e0af
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://nice-wave-0f618f503-843.westeurope.azurestaticapps.net |
Before creating a PR, please check that:
yarn test
)