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

#143 light executed script #163

Merged
merged 8 commits into from
Oct 19, 2021
Merged

#143 light executed script #163

merged 8 commits into from
Oct 19, 2021

Conversation

asolovieff
Copy link
Contributor

#143
Add LightExecutedScript that only contains the name, checksum and identifier.
DatamaintainDriver.listExecutedScripts() now return a sequence of LightExecutedScripts instead of the full ExecutedScript.
Modified MongoDriver, JdbcDriver and tests accordingly.
@driccio

@asolovieff asolovieff requested a review from driccio September 21, 2021 12:00
@asolovieff asolovieff requested review from Lysoun and nroulon October 5, 2021 07:10
Copy link
Member

@Lysoun Lysoun left a comment

Choose a reason for hiding this comment

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

I'm not so sure about reading the identifiers. They are used during the scripts execution to determine in what order the scripts should be played but are there relevant otherwise?
Since the objective here is to serialize ExecutedScript and only read LightExecutedScript, I think the code to map executed scripts db to executed scripts could be removed and no code to serialize LightExecutedScript is necessary.

script3,
script2
lightScript3,
lightScript2
Copy link
Member

Choose a reason for hiding this comment

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

Since ExecutedScript extends LightExecutedScript, shouldn't it work without even without giving explicit LightExecutedScript? Did you try?

Copy link
Member

Choose a reason for hiding this comment

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

If it does not, maybe add an extension to ExecutedScript in the tests that creates the light executed script associated to a script (basically by casting it I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it does not work without explicit type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And casting is not enough.

Copy link
Member

@Lysoun Lysoun Oct 5, 2021

Choose a reason for hiding this comment

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

Then you could add a function extension to ExecutedScript, toLightExecutedScript(), in the tests. I'm sure it will help for other tests because MongoDriverTest should also check stuff like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already done

executionStatus,
action,
executionDurationInMillis,
executionOutput
Copy link
Member

Choose a reason for hiding this comment

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

Is this mapping function still necessary? It appears to me that only light executed scripts will be read from the database from now on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed

name = name,
checksum = checksum,
identifier = identifier
)
Copy link
Member

Choose a reason for hiding this comment

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

Is this function necessary? Light executed scripts should not be inserted in the database

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed

override fun serializeLightExecutedScript(lightExecutedScript: LightExecutedScript): String {
val lightExecutedScriptDb = lightExecutedScript.toLightExecutedScriptDb()

return mapper.stringify(LightExecutedScriptDb.serializer(), lightExecutedScriptDb)
Copy link
Member

Choose a reason for hiding this comment

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

Are light executed scripts serialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No they are not.

Add an extension function toLightExecutedScript() inside test to avoid lightScript variable in addition to script variable.
ExecutedScript are only serialized and LightExecutedScript are only deserialized.
@github-actions
Copy link

github-actions bot commented Oct 5, 2021

Unit Test Results

  46 files  ±0    46 suites  ±0   13s ⏱️ ±0s
153 tests ±0  153 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 3889012. ± Comparison against base commit 17ba0ef.

Copy link
Member

@driccio driccio left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR.

@asolovieff asolovieff merged commit de88ace into dev Oct 19, 2021
@asolovieff asolovieff deleted the #143-LightExecutedScript branch October 19, 2021 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants