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

Fixes #23830: Migrate users and supervised target APIs in change-validation to zio-json #614

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Dec 4, 2023

https://issues.rudder.io/issues/23830

This is the migration of 2 APIs in the change-validation plugin to use zio-json to replace lift-json (there is one bigger REST API to migrate still, and in consequence lots of refactoring from the lift Box type to the zio IOResult type in the interfaces, but this will be made in another PR).

I kept the same behavior everywhere as I could, so there may be 'weird' (rather uncommon) things such as logging a decoding error...
I also added tests to the APIs using the yaml format, and to the repositories, which had some saving 'workflow' to test...

@clarktsiory clarktsiory requested a review from fanf December 4, 2023 08:35
@clarktsiory
Copy link
Contributor Author

Commit modified

@clarktsiory clarktsiory force-pushed the arch_23830/migrate_users_and_supervised_target_apis_in_change_validation_to_zio_json branch from 84730d8 to df34155 Compare December 4, 2023 08:36
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit


@RunWith(classOf[JUnitRunner])
class MigrateSupervisedGroupsTests extends Specification with JsonSpecMatcher with AfterAll {
isolated
Copy link
Member

Choose a reason for hiding this comment

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

it needs to be just sequential, not isolated, else the whole set up is done again, which is costly and can lead to error depending of the millesecond of directory creation, for ex:

image

Copy link
Member

Choose a reason for hiding this comment

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

(previously I got a "directory already exist", which was more explicit)


"do nothing if the directory is not writable" in {
val unsupervisedRepo = new UnsupervisedTargetsRepository(tmpDirPath, "unsupervised-targets.json")
tmpDir.setPermissions(PosixFilePermissions.fromString("r--r--r--").asScala.toSet)
Copy link
Member

Choose a reason for hiding this comment

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

this need to be at least rw-r--r-- else the test modifying it fails. (SupervisedTargetsApiTest, the POST)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right the tests were not passing ! The change was there :

.setPermissions(PosixFilePermissions.fromString("rw-r--r--").asScala.toSet)
(I think I changed it just to see the response I forgot to revert back to rw 😄)

@clarktsiory clarktsiory force-pushed the arch_23830/migrate_users_and_supervised_target_apis_in_change_validation_to_zio_json branch from 50c26a1 to 31df6b8 Compare December 13, 2023 11:21
Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

LGTM
(more tests will be needed once rudder header are fully restored)

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder-plugins/pull/614
-- Your faithful QA
Kant merge: "In law a man is guilty when he violates the rights of others. In ethics he is guilty if he only thinks of doing so."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/77567/console)

@fanf
Copy link
Member

fanf commented Dec 15, 2023

OK, squash merging this PR

@fanf fanf closed this Dec 15, 2023
@fanf fanf force-pushed the arch_23830/migrate_users_and_supervised_target_apis_in_change_validation_to_zio_json branch from 31df6b8 to eb13a14 Compare December 15, 2023 20:30
@fanf fanf merged commit eb13a14 into Normation:master Dec 15, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants