-
Notifications
You must be signed in to change notification settings - Fork 73
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 #15265: Migrate ncf delete api and improve workflow #2333
Fixes #15265: Migrate ncf delete api and improve workflow #2333
Conversation
5916cfb
to
8288ce9
Compare
Commit modified |
cr <- ((d.accumulate { directive => | ||
val tName = TechniqueName(techniqueName) | ||
for { | ||
a <- readDirectives.getDirectiveWithContext(directive.id).notOptional(s"Could not find Directive ${directive.id.value}") |
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 think that you should keep the getFullDirectiveLibrary
and avoid to request here again. It will avoid ldap requests
|
||
|
||
} else | ||
Left(Unexpected(s"${d.size} directives are defined for ${techniqueName}/${techniqueVersion} please delete them, or force deletion")).toIO |
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.
You can do Unexpected(....).succeed
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.
.fail
of course, not succeed here :/
webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/ncf/TechniqueWriter.scala
Show resolved
Hide resolved
for { | ||
d <- readDirectives.getFullDirectiveLibrary().map(_.allActiveTechniques.values.filter(_.techniqueName.value == techniqueName).flatMap(_.directives).filter(_.techniqueVersion.toString == techniqueVersion)) | ||
_ <- (d match { | ||
case Nil => Right(d).toIO |
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.
d.succeed
Commit modified |
cr <- directives.map(createCr(_,technique.rootSection)).reduceOption(mergeCrs).notOptional(s"Could not create a change request to delete ${techniqueName}/${techniqueVersion} directives") | ||
_ <- wf.startWorkflow(cr, committer, Some(s"Deleting technique ${techniqueName}/${techniqueVersion}")).succeed | ||
} yield { | ||
cr |
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.
you should yield ()
if the result is not used.
|
||
technique <- techniqueRepository.get(techniqueId).notOptional(s"No Technique with ID '${techniqueId.toString()}' found in reference library.") | ||
_ <- directives match { | ||
case Nil => directives.succeed |
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 shoud be UIO.unit
if you don't use the result type
} | ||
_ <- archiver.deleteTechnique(techniqueName,techniqueVersion,modId,committer, s"Deleting technique ${techniqueName}/${techniqueVersion}") | ||
|
||
libUpdate <- techLibUpdate.update(modId, committer, Some(s"Update Technique library after deletion of Technique ${techniqueName}")).succeed.chainError(s"An error occurred during technique update after deletion of Technique ${techniqueName}") |
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 you don't need libUpdate
ident <- personIdentservice.getPersonIdentOrDefault(commiter.name) | ||
rm <- IOResult.effect(git.rm.addFilepattern(s"techniques/ncf_techniques/${techniqueName}/${techniqueVersion}").call()) | ||
|
||
commit <- IOResult.effect(git.commit.setCommitter(ident).setMessage(msg).call()) |
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 you don't need commit
Commit modified |
This PR is not mergeable to upper versions. |
OK, squash merging this PR |
5d27ecf
to
99a69f8
Compare
https://issues.rudder.io/issues/15265