From d8545c48d0c6de35850db14f846f2c21636ca64d Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 11:45:58 +0100 Subject: [PATCH 01/15] step 1 : port JGitRepositoryTest to ZIO Test --- .../services/JGitRepositoryTest2.scala | 279 ++++++++++++++++++ 1 file changed, 279 insertions(+) create mode 100644 webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala new file mode 100644 index 0000000000..17380fa589 --- /dev/null +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala @@ -0,0 +1,279 @@ +/* + ************************************************************************************* + * Copyright 2021 Normation SAS + ************************************************************************************* + * + * This file is part of Rudder. + * + * Rudder is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * In accordance with the terms of section 7 (7. Additional Terms.) of + * the GNU General Public License version 3, the copyright holders add + * the following Additional permissions: + * Notwithstanding to the terms of section 5 (5. Conveying Modified Source + * Versions) and 6 (6. Conveying Non-Source Forms.) of the GNU General + * Public License version 3, when you create a Related Module, this + * Related Module is not considered as a part of the work and may be + * distributed under the license agreement of your choice. + * A "Related Module" means a set of sources files including their + * documentation that, without modification of the Source Code, enables + * supplementary functions or services in addition to those offered by + * the Software. + * + * Rudder is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Rudder. If not, see . + + * + ************************************************************************************* + */ + +package com.normation.cfclerk.services + +import better.files.File +import com.normation.cfclerk.domain.TechniqueCategoryMetadata +import com.normation.cfclerk.services.impl.SystemVariableSpecServiceImpl +import com.normation.cfclerk.xmlparsers.SectionSpecParser +import com.normation.cfclerk.xmlparsers.TechniqueParser +import com.normation.cfclerk.xmlparsers.VariableSpecParser +import com.normation.errors +import com.normation.errors.Inconsistency +import com.normation.errors.IOResult +import com.normation.errors.RudderError +import com.normation.errors.effectUioUnit +import com.normation.eventlog.EventActor +import com.normation.eventlog.ModificationId +import com.normation.rudder.db.DB +import com.normation.rudder.git.GitCommitId +import com.normation.rudder.git.GitConfigItemRepository +import com.normation.rudder.git.GitRepositoryProvider +import com.normation.rudder.git.GitRepositoryProviderImpl +import com.normation.rudder.ncf.EditorTechnique +import com.normation.rudder.ncf.TechniqueCompilationOutput +import com.normation.rudder.ncf.TechniqueCompiler +import com.normation.rudder.ncf.TechniqueCompilerApp +import com.normation.rudder.repository.GitModificationRepository +import com.normation.rudder.repository.xml.RudderPrettyPrinter +import com.normation.rudder.repository.xml.TechniqueArchiverImpl +import com.normation.rudder.repository.xml.XmlArchiverUtils +import com.normation.rudder.services.user.TrivialPersonIdentService +import org.eclipse.jgit.lib.PersonIdent +import org.eclipse.jgit.lib.Repository +import org.eclipse.jgit.revwalk.RevWalk +import org.eclipse.jgit.treewalk.TreeWalk +import scala.util.Random +import zio.Chunk +import zio.IO +import zio.System +import zio.ZIO +import zio.ZLayer +import zio.syntax.ToZio +import zio.test.* +import zio.test.Assertion.* + +object JGitRepositoryTest2 extends ZIOSpecDefault { + + val prettyPrinter: RudderPrettyPrinter = new RudderPrettyPrinter(Int.MaxValue, 2) + + val modRepo: GitModificationRepository = new GitModificationRepository { + override def getCommits(modificationId: ModificationId): IOResult[Option[GitCommitId]] = None.succeed + override def addCommit(commit: GitCommitId, modId: ModificationId): IOResult[DB.GitCommitJoin] = + DB.GitCommitJoin(commit, modId).succeed + } + + val actor = new PersonIdent("test", "test@test.com") + + val personIdent: TrivialPersonIdentService = new TrivialPersonIdentService() + + val techniqueParser: TechniqueParser = { + val varParser = new VariableSpecParser + new TechniqueParser(varParser, new SectionSpecParser(varParser), new SystemVariableSpecServiceImpl()) + } + + val techniqueCompiler = new TechniqueCompiler { + override def compileTechnique(technique: EditorTechnique): IOResult[TechniqueCompilationOutput] = { + TechniqueCompilationOutput(TechniqueCompilerApp.Rudderc, 0, Chunk.empty, "", "", "").succeed + } + + override def getCompilationOutputFile(technique: EditorTechnique): File = File("compilation-config.yml") + + override def getCompilationConfigFile(technique: EditorTechnique): File = File("compilation-output.yml") + } + + case class TempDir(path: File) + + object TempDir { + + /** + * Add a switch to be able to see tmp files (not clean temps) with + * -Dtests.clean.tmp=false + */ + val layer: ZLayer[Any, Nothing, TempDir] = ZLayer.scoped { + for { + keepFiles <- System.property("tests.clean.tmp").map(_.contains("false")).orDie + tempDir <- ZIO + .attemptBlockingIO(File.newTemporaryDirectory(prefix = "test-jgit-")) + .withFinalizer { dir => + (ZIO.logInfo(s"Deleting directory ${dir.path}") *> + ZIO.attemptBlocking(dir.delete(swallowIOExceptions = true))) + .unless(keepFiles) + .orDie + } + .orDie + } yield TempDir(tempDir) + } + } + + // listing files at a commit is complicated + def readElementsAt(repository: Repository, commit: String): ZIO[Any, errors.SystemError, List[String]] = { + val ref = repository.findRef(commit) + + // a RevWalk allows to walk over commits based on some filtering that is defined + val walkM = ZIO.acquireRelease(IOResult.attempt(new RevWalk(repository)))(x => effectUioUnit(x.close())) + val treeWalkM = ZIO.acquireRelease(IOResult.attempt(new TreeWalk(repository)))(x => effectUioUnit(x.close())) + + ZIO.scoped[Any]( + walkM.flatMap(walk => { + for { + commit <- IOResult.attempt(walk.parseCommit(ref.getObjectId)) + tree <- IOResult.attempt(commit.getTree) + res <- ZIO.scoped[Any](treeWalkM.flatMap { treeWalk => + treeWalk.setRecursive(true) // ok, can't throw exception + + IOResult.attempt(treeWalk.addTree(tree)) *> + ZIO.loop(treeWalk)(_.next, identity)(x => IOResult.attempt(x.getPathString)) + }) + } yield res + }) + ) + } + + val category = TechniqueCategoryMetadata("My new category", "A new category", isSystem = false) + val catPath = List("systemSettings", "myNewCategory") + + val modId = new ModificationId("add-technique-cat") + + def makeRepo(gitRoot: TempDir) = GitRepositoryProviderImpl.make(gitRoot.path.pathAsString) + + // for test, we use as a group owner whatever git root directory has + def currentUserName(repo: GitRepositoryProviderImpl): String = repo.rootDirectory.groupName + + val spec: Spec[Any, RudderError] = suite("The test lib")( + // to assess the usefulness of semaphore, you can remove `gitRepo.semaphore.withPermit` + // in `commitAddFile` to check that you get the JGitInternalException. + // More advanced tests may be needed to handle more complex cases of concurrent access, + // see: https://issues.rudder.io/issues/19910 + test("not throw JGitInternalError on concurrent write") { + + def getName(length: Int): IO[errors.RudderError, String] = { + if (length < 1) Inconsistency("Length must be positive").fail + else { + IOResult.attempt("")(Random.alphanumeric.take(length).toList.mkString("")) + } + } + + def add(i: Int)(gitRoot: TempDir, archive: GitConfigItemRepository with XmlArchiverUtils) = (for { + name <- getName(8).map(s => i.toString + "_" + s) + file = gitRoot.path / name + f <- IOResult.attempt(file.write("something in " + name)) + _ <- archive.commitAddFileWithModId(ModificationId(name), actor, name, "add " + name) + } yield (name)) + + def makeArchive(repository: GitRepositoryProviderImpl): GitConfigItemRepository with XmlArchiverUtils = { + new GitConfigItemRepository with XmlArchiverUtils { + override val gitRepo: GitRepositoryProvider = repository + override def relativePath: String = "" + override def xmlPrettyPrinter = prettyPrinter + override def encoding: String = "UTF-8" + override def gitModificationRepository: GitModificationRepository = modRepo + override def groupOwner: String = currentUserName(repository) + } + } + + for { + gitRoot <- ZIO.service[TempDir] + repo <- makeRepo(gitRoot) + archive = makeArchive(repo) + files <- ZIO.foreachPar(1 to 50)(i => add(i)(gitRoot, archive)).withParallelism(16) + created <- readElementsAt(repo.db, "refs/heads/master") + } yield assert(created)(hasSameElements(files)) + + }.provide(TempDir.layer), + suite("save a category")( + test("create a new file and commit if the category does not exist") { + for { + _ <- ZIO.debug("test 1 started") + gitRoot <- ZIO.service[TempDir] + repo <- makeRepo(gitRoot) + techniqueArchive = new TechniqueArchiverImpl( + gitRepo = repo, + xmlPrettyPrinter = prettyPrinter, + gitModificationRepository = modRepo, + personIdentservice = personIdent, + techniqueParser = techniqueParser, + techniqueCompiler = techniqueCompiler, + groupOwner = currentUserName(repo) + ) + _ <- techniqueArchive + .saveTechniqueCategory( + catPath, + category, + modId, + EventActor("test"), + s"test: commit add category ${catPath.mkString("/")}" + ) + catFile = repo.rootDirectory / "techniques" / "systemSettings" / "myNewCategory" / "category.xml" + xml = catFile.contentAsString + lastCommitMsg = repo.git.log().setMaxCount(1).call().iterator().next().getFullMessage + } yield { + // note: no false ; it's only written when true + assert(xml)( + equalTo( + """ + | My new category + | A new category + |""".stripMargin + ) + ) && + assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) + } + }, + test("does nothing when the category already exists") { + for { + _ <- ZIO.debug("test 2 started") + gitRoot <- ZIO.service[TempDir] + repo <- makeRepo(gitRoot) + techniqueArchive = new TechniqueArchiverImpl( + gitRepo = repo, + xmlPrettyPrinter = prettyPrinter, + gitModificationRepository = modRepo, + personIdentservice = personIdent, + techniqueParser = techniqueParser, + techniqueCompiler = techniqueCompiler, + groupOwner = currentUserName(repo) + ) + + _ <- techniqueArchive + .saveTechniqueCategory( + catPath, + category, + modId, + EventActor("test"), + s"test: commit add category ${catPath.mkString("/")}" + ) + + _ <- techniqueArchive.saveTechniqueCategory(catPath, category, modId, EventActor("test"), s"test: commit again") + lastCommitMsg = repo.git.log().setMaxCount(1).call().iterator().next().getFullMessage + } yield assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) + } + ).provideShared(TempDir.layer) @@ TestAspect.sequential + ) + +} From d797c1e10bb74bc50e7a9be698ee18d4c9ff7a43 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 12:01:59 +0100 Subject: [PATCH 02/15] step 2 : make no assumption about service stateful-ness --- .../services/JGitRepositoryTest2.scala | 111 +++++++++--------- 1 file changed, 58 insertions(+), 53 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala index 17380fa589..9bca773ad3 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala @@ -78,56 +78,56 @@ import zio.syntax.ToZio import zio.test.* import zio.test.Assertion.* +case class TempDir(path: File) + +object TempDir { + + /** + * Add a switch to be able to see tmp files (not clean temps) with + * -Dtests.clean.tmp=false + */ + val layer: ZLayer[Any, Nothing, TempDir] = ZLayer.scoped { + for { + keepFiles <- System.property("tests.clean.tmp").map(_.contains("false")).orDie + tempDir <- ZIO + .attemptBlockingIO(File.newTemporaryDirectory(prefix = "test-jgit-")) + .withFinalizer { dir => + (ZIO.logInfo(s"Deleting directory ${dir.path}") *> + ZIO.attemptBlocking(dir.delete(swallowIOExceptions = true))) + .unless(keepFiles) + .orDie + } + .orDie + } yield TempDir(tempDir) + } +} + object JGitRepositoryTest2 extends ZIOSpecDefault { - val prettyPrinter: RudderPrettyPrinter = new RudderPrettyPrinter(Int.MaxValue, 2) + def prettyPrinter: RudderPrettyPrinter = new RudderPrettyPrinter(Int.MaxValue, 2) - val modRepo: GitModificationRepository = new GitModificationRepository { - override def getCommits(modificationId: ModificationId): IOResult[Option[GitCommitId]] = None.succeed - override def addCommit(commit: GitCommitId, modId: ModificationId): IOResult[DB.GitCommitJoin] = - DB.GitCommitJoin(commit, modId).succeed + object StubGitModificationRepository { + def make: GitModificationRepository = new GitModificationRepository { + override def getCommits(modificationId: ModificationId): IOResult[Option[GitCommitId]] = None.succeed + override def addCommit(commit: GitCommitId, modId: ModificationId): IOResult[DB.GitCommitJoin] = + DB.GitCommitJoin(commit, modId).succeed + } } - val actor = new PersonIdent("test", "test@test.com") - - val personIdent: TrivialPersonIdentService = new TrivialPersonIdentService() - - val techniqueParser: TechniqueParser = { + def makeTechniqueParser: TechniqueParser = { val varParser = new VariableSpecParser new TechniqueParser(varParser, new SectionSpecParser(varParser), new SystemVariableSpecServiceImpl()) } - val techniqueCompiler = new TechniqueCompiler { - override def compileTechnique(technique: EditorTechnique): IOResult[TechniqueCompilationOutput] = { - TechniqueCompilationOutput(TechniqueCompilerApp.Rudderc, 0, Chunk.empty, "", "", "").succeed - } - - override def getCompilationOutputFile(technique: EditorTechnique): File = File("compilation-config.yml") - - override def getCompilationConfigFile(technique: EditorTechnique): File = File("compilation-output.yml") - } - - case class TempDir(path: File) + object StubbedTechniqueCompiler { + def make: TechniqueCompiler = new TechniqueCompiler { + override def compileTechnique(technique: EditorTechnique): IOResult[TechniqueCompilationOutput] = { + TechniqueCompilationOutput(TechniqueCompilerApp.Rudderc, 0, Chunk.empty, "", "", "").succeed + } - object TempDir { + override def getCompilationOutputFile(technique: EditorTechnique): File = File("compilation-config.yml") - /** - * Add a switch to be able to see tmp files (not clean temps) with - * -Dtests.clean.tmp=false - */ - val layer: ZLayer[Any, Nothing, TempDir] = ZLayer.scoped { - for { - keepFiles <- System.property("tests.clean.tmp").map(_.contains("false")).orDie - tempDir <- ZIO - .attemptBlockingIO(File.newTemporaryDirectory(prefix = "test-jgit-")) - .withFinalizer { dir => - (ZIO.logInfo(s"Deleting directory ${dir.path}") *> - ZIO.attemptBlocking(dir.delete(swallowIOExceptions = true))) - .unless(keepFiles) - .orDie - } - .orDie - } yield TempDir(tempDir) + override def getCompilationConfigFile(technique: EditorTechnique): File = File("compilation-output.yml") } } @@ -158,7 +158,7 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { val category = TechniqueCategoryMetadata("My new category", "A new category", isSystem = false) val catPath = List("systemSettings", "myNewCategory") - val modId = new ModificationId("add-technique-cat") + val modId = ModificationId("add-technique-cat") def makeRepo(gitRoot: TempDir) = GitRepositoryProviderImpl.make(gitRoot.path.pathAsString) @@ -179,14 +179,18 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { } } - def add(i: Int)(gitRoot: TempDir, archive: GitConfigItemRepository with XmlArchiverUtils) = (for { + def add(i: Int)(gitRoot: TempDir, archive: GitConfigItemRepository with XmlArchiverUtils) = for { name <- getName(8).map(s => i.toString + "_" + s) file = gitRoot.path / name - f <- IOResult.attempt(file.write("something in " + name)) + _ <- IOResult.attempt(file.write("something in " + name)) + actor = new PersonIdent("test", "test@test.com") _ <- archive.commitAddFileWithModId(ModificationId(name), actor, name, "add " + name) - } yield (name)) + } yield name - def makeArchive(repository: GitRepositoryProviderImpl): GitConfigItemRepository with XmlArchiverUtils = { + def makeArchive( + repository: GitRepositoryProviderImpl, + modRepo: GitModificationRepository + ): GitConfigItemRepository with XmlArchiverUtils = { new GitConfigItemRepository with XmlArchiverUtils { override val gitRepo: GitRepositoryProvider = repository override def relativePath: String = "" @@ -200,7 +204,8 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { for { gitRoot <- ZIO.service[TempDir] repo <- makeRepo(gitRoot) - archive = makeArchive(repo) + modRepo = StubGitModificationRepository.make + archive = makeArchive(repo, modRepo) files <- ZIO.foreachPar(1 to 50)(i => add(i)(gitRoot, archive)).withParallelism(16) created <- readElementsAt(repo.db, "refs/heads/master") } yield assert(created)(hasSameElements(files)) @@ -215,10 +220,10 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { techniqueArchive = new TechniqueArchiverImpl( gitRepo = repo, xmlPrettyPrinter = prettyPrinter, - gitModificationRepository = modRepo, - personIdentservice = personIdent, - techniqueParser = techniqueParser, - techniqueCompiler = techniqueCompiler, + gitModificationRepository = StubGitModificationRepository.make, + personIdentservice = new TrivialPersonIdentService(), + techniqueParser = makeTechniqueParser, + techniqueCompiler = StubbedTechniqueCompiler.make, groupOwner = currentUserName(repo) ) _ <- techniqueArchive @@ -253,10 +258,10 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { techniqueArchive = new TechniqueArchiverImpl( gitRepo = repo, xmlPrettyPrinter = prettyPrinter, - gitModificationRepository = modRepo, - personIdentservice = personIdent, - techniqueParser = techniqueParser, - techniqueCompiler = techniqueCompiler, + gitModificationRepository = StubGitModificationRepository.make, + personIdentservice = new TrivialPersonIdentService(), + techniqueParser = makeTechniqueParser, + techniqueCompiler = StubbedTechniqueCompiler.make, groupOwner = currentUserName(repo) ) From f61ff60303feadc1453e593f63775a62ff312a43 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 12:12:53 +0100 Subject: [PATCH 03/15] step 3 : use a few zlayer to help build test env --- .../services/JGitRepositoryTest2.scala | 167 +++++++++++------- 1 file changed, 101 insertions(+), 66 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala index 9bca773ad3..335352814f 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala @@ -72,6 +72,7 @@ import scala.util.Random import zio.Chunk import zio.IO import zio.System +import zio.ULayer import zio.ZIO import zio.ZLayer import zio.syntax.ToZio @@ -107,30 +108,42 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { def prettyPrinter: RudderPrettyPrinter = new RudderPrettyPrinter(Int.MaxValue, 2) object StubGitModificationRepository { - def make: GitModificationRepository = new GitModificationRepository { - override def getCommits(modificationId: ModificationId): IOResult[Option[GitCommitId]] = None.succeed - override def addCommit(commit: GitCommitId, modId: ModificationId): IOResult[DB.GitCommitJoin] = - DB.GitCommitJoin(commit, modId).succeed + val layer: ULayer[GitModificationRepository] = ZLayer.succeed { + new GitModificationRepository { + override def getCommits(modificationId: ModificationId): IOResult[Option[GitCommitId]] = None.succeed + + override def addCommit(commit: GitCommitId, modId: ModificationId): IOResult[DB.GitCommitJoin] = + DB.GitCommitJoin(commit, modId).succeed + } } } - def makeTechniqueParser: TechniqueParser = { + val techniqueParserLayer: ULayer[TechniqueParser] = ZLayer.succeed { val varParser = new VariableSpecParser new TechniqueParser(varParser, new SectionSpecParser(varParser), new SystemVariableSpecServiceImpl()) } object StubbedTechniqueCompiler { - def make: TechniqueCompiler = new TechniqueCompiler { - override def compileTechnique(technique: EditorTechnique): IOResult[TechniqueCompilationOutput] = { - TechniqueCompilationOutput(TechniqueCompilerApp.Rudderc, 0, Chunk.empty, "", "", "").succeed - } + val layer: ULayer[TechniqueCompiler] = ZLayer.succeed { + new TechniqueCompiler { + override def compileTechnique(technique: EditorTechnique): IOResult[TechniqueCompilationOutput] = { + TechniqueCompilationOutput(TechniqueCompilerApp.Rudderc, 0, Chunk.empty, "", "", "").succeed + } - override def getCompilationOutputFile(technique: EditorTechnique): File = File("compilation-config.yml") + override def getCompilationOutputFile(technique: EditorTechnique): File = File("compilation-config.yml") - override def getCompilationConfigFile(technique: EditorTechnique): File = File("compilation-output.yml") + override def getCompilationConfigFile(technique: EditorTechnique): File = File("compilation-output.yml") + } } } + val gitRepositoryProviderLayer: ZLayer[TempDir, RudderError, GitRepositoryProviderImpl] = ZLayer { + for { + gitRoot <- ZIO.service[TempDir] + result <- GitRepositoryProviderImpl.make(gitRoot.path.pathAsString) + } yield result + } + // listing files at a commit is complicated def readElementsAt(repository: Repository, commit: String): ZIO[Any, errors.SystemError, List[String]] = { val ref = repository.findRef(commit) @@ -160,10 +173,16 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { val modId = ModificationId("add-technique-cat") - def makeRepo(gitRoot: TempDir) = GitRepositoryProviderImpl.make(gitRoot.path.pathAsString) + case class GroupOwner(value: String) // for test, we use as a group owner whatever git root directory has - def currentUserName(repo: GitRepositoryProviderImpl): String = repo.rootDirectory.groupName + def currentUserName(repo: GitRepositoryProviderImpl): GroupOwner = GroupOwner(repo.rootDirectory.groupName) + + val currentUserNameLayer: ZLayer[GitRepositoryProviderImpl, Nothing, GroupOwner] = ZLayer { + for { + repo <- ZIO.service[GitRepositoryProviderImpl] + } yield currentUserName(repo) + } val spec: Spec[Any, RudderError] = suite("The test lib")( // to assess the usefulness of semaphore, you can remove `gitRepo.semaphore.withPermit` @@ -197,46 +216,51 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { override def xmlPrettyPrinter = prettyPrinter override def encoding: String = "UTF-8" override def gitModificationRepository: GitModificationRepository = modRepo - override def groupOwner: String = currentUserName(repository) + override def groupOwner: String = currentUserName(repository).value } } for { - gitRoot <- ZIO.service[TempDir] - repo <- makeRepo(gitRoot) - modRepo = StubGitModificationRepository.make - archive = makeArchive(repo, modRepo) + gitRoot <- ZIO.service[TempDir] + modRepo <- ZIO.service[GitModificationRepository] + gitRepositoryProvider <- ZIO.service[GitRepositoryProviderImpl] + + archive = makeArchive(gitRepositoryProvider, modRepo) files <- ZIO.foreachPar(1 to 50)(i => add(i)(gitRoot, archive)).withParallelism(16) - created <- readElementsAt(repo.db, "refs/heads/master") + created <- readElementsAt(gitRepositoryProvider.db, "refs/heads/master") } yield assert(created)(hasSameElements(files)) - }.provide(TempDir.layer), + }.provide(TempDir.layer, StubGitModificationRepository.layer, gitRepositoryProviderLayer), suite("save a category")( test("create a new file and commit if the category does not exist") { for { - _ <- ZIO.debug("test 1 started") - gitRoot <- ZIO.service[TempDir] - repo <- makeRepo(gitRoot) - techniqueArchive = new TechniqueArchiverImpl( - gitRepo = repo, - xmlPrettyPrinter = prettyPrinter, - gitModificationRepository = StubGitModificationRepository.make, - personIdentservice = new TrivialPersonIdentService(), - techniqueParser = makeTechniqueParser, - techniqueCompiler = StubbedTechniqueCompiler.make, - groupOwner = currentUserName(repo) - ) - _ <- techniqueArchive - .saveTechniqueCategory( - catPath, - category, - modId, - EventActor("test"), - s"test: commit add category ${catPath.mkString("/")}" - ) - catFile = repo.rootDirectory / "techniques" / "systemSettings" / "myNewCategory" / "category.xml" - xml = catFile.contentAsString - lastCommitMsg = repo.git.log().setMaxCount(1).call().iterator().next().getFullMessage + _ <- ZIO.debug("test 1 started") + modRepo <- ZIO.service[GitModificationRepository] + techniqueParse <- ZIO.service[TechniqueParser] + techniqueCompiler <- ZIO.service[TechniqueCompiler] + gitRepositoryProvider <- ZIO.service[GitRepositoryProviderImpl] + personIdentservice <- ZIO.service[TrivialPersonIdentService] + groupOwner <- ZIO.service[GroupOwner] + techniqueArchive = new TechniqueArchiverImpl( + gitRepo = gitRepositoryProvider, + xmlPrettyPrinter = prettyPrinter, + gitModificationRepository = modRepo, + personIdentservice = personIdentservice, + techniqueParser = techniqueParse, + techniqueCompiler = techniqueCompiler, + groupOwner = groupOwner.value + ) + _ <- techniqueArchive + .saveTechniqueCategory( + catPath, + category, + modId, + EventActor("test"), + s"test: commit add category ${catPath.mkString("/")}" + ) + catFile = gitRepositoryProvider.rootDirectory / "techniques" / "systemSettings" / "myNewCategory" / "category.xml" + xml = catFile.contentAsString + lastCommitMsg = gitRepositoryProvider.git.log().setMaxCount(1).call().iterator().next().getFullMessage } yield { // note: no false ; it's only written when true assert(xml)( @@ -252,33 +276,44 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { }, test("does nothing when the category already exists") { for { - _ <- ZIO.debug("test 2 started") - gitRoot <- ZIO.service[TempDir] - repo <- makeRepo(gitRoot) - techniqueArchive = new TechniqueArchiverImpl( - gitRepo = repo, - xmlPrettyPrinter = prettyPrinter, - gitModificationRepository = StubGitModificationRepository.make, - personIdentservice = new TrivialPersonIdentService(), - techniqueParser = makeTechniqueParser, - techniqueCompiler = StubbedTechniqueCompiler.make, - groupOwner = currentUserName(repo) - ) - - _ <- techniqueArchive - .saveTechniqueCategory( - catPath, - category, - modId, - EventActor("test"), - s"test: commit add category ${catPath.mkString("/")}" - ) + _ <- ZIO.debug("test 2 started") + modRepo <- ZIO.service[GitModificationRepository] + techniqueParse <- ZIO.service[TechniqueParser] + techniqueCompiler <- ZIO.service[TechniqueCompiler] + gitRepositoryProvider <- ZIO.service[GitRepositoryProviderImpl] + personIdentservice <- ZIO.service[TrivialPersonIdentService] + groupOwner <- ZIO.service[GroupOwner] + techniqueArchive = new TechniqueArchiverImpl( + gitRepo = gitRepositoryProvider, + xmlPrettyPrinter = prettyPrinter, + gitModificationRepository = modRepo, + personIdentservice = personIdentservice, + techniqueParser = techniqueParse, + techniqueCompiler = techniqueCompiler, + groupOwner = groupOwner.value + ) + _ <- techniqueArchive + .saveTechniqueCategory( + catPath, + category, + modId, + EventActor("test"), + s"test: commit add category ${catPath.mkString("/")}" + ) _ <- techniqueArchive.saveTechniqueCategory(catPath, category, modId, EventActor("test"), s"test: commit again") - lastCommitMsg = repo.git.log().setMaxCount(1).call().iterator().next().getFullMessage + lastCommitMsg = gitRepositoryProvider.git.log().setMaxCount(1).call().iterator().next().getFullMessage } yield assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) } - ).provideShared(TempDir.layer) @@ TestAspect.sequential + ).provideShared( + TempDir.layer, + StubGitModificationRepository.layer, + techniqueParserLayer, + StubbedTechniqueCompiler.layer, + gitRepositoryProviderLayer, + ZLayer.succeed(new TrivialPersonIdentService()), + currentUserNameLayer + ) @@ TestAspect.sequential ) } From b1d289d9f1e8e29af1237875be186de791d063ff Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 12:23:22 +0100 Subject: [PATCH 04/15] step 4: system-under-test instantiation is not interesting in test cases code --- .../services/JGitRepositoryTest2.scala | 57 +++++++++---------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala index 335352814f..223bda85f7 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala @@ -184,6 +184,30 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { } yield currentUserName(repo) } + val techniqueArchiverLayer: ZLayer[ + GroupOwner & TrivialPersonIdentService & GitRepositoryProviderImpl & TechniqueCompiler & TechniqueParser & GitModificationRepository, + Nothing, + TechniqueArchiverImpl + ] = ZLayer { + for { + modRepo <- ZIO.service[GitModificationRepository] + techniqueParse <- ZIO.service[TechniqueParser] + techniqueCompiler <- ZIO.service[TechniqueCompiler] + gitRepositoryProvider <- ZIO.service[GitRepositoryProviderImpl] + personIdentservice <- ZIO.service[TrivialPersonIdentService] + groupOwner <- ZIO.service[GroupOwner] + techniqueArchive = new TechniqueArchiverImpl( + gitRepo = gitRepositoryProvider, + xmlPrettyPrinter = prettyPrinter, + gitModificationRepository = modRepo, + personIdentservice = personIdentservice, + techniqueParser = techniqueParse, + techniqueCompiler = techniqueCompiler, + groupOwner = groupOwner.value + ) + } yield techniqueArchive + } + val spec: Spec[Any, RudderError] = suite("The test lib")( // to assess the usefulness of semaphore, you can remove `gitRepo.semaphore.withPermit` // in `commitAddFile` to check that you get the JGitInternalException. @@ -235,21 +259,8 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { test("create a new file and commit if the category does not exist") { for { _ <- ZIO.debug("test 1 started") - modRepo <- ZIO.service[GitModificationRepository] - techniqueParse <- ZIO.service[TechniqueParser] - techniqueCompiler <- ZIO.service[TechniqueCompiler] gitRepositoryProvider <- ZIO.service[GitRepositoryProviderImpl] - personIdentservice <- ZIO.service[TrivialPersonIdentService] - groupOwner <- ZIO.service[GroupOwner] - techniqueArchive = new TechniqueArchiverImpl( - gitRepo = gitRepositoryProvider, - xmlPrettyPrinter = prettyPrinter, - gitModificationRepository = modRepo, - personIdentservice = personIdentservice, - techniqueParser = techniqueParse, - techniqueCompiler = techniqueCompiler, - groupOwner = groupOwner.value - ) + techniqueArchive <- ZIO.service[TechniqueArchiverImpl] _ <- techniqueArchive .saveTechniqueCategory( catPath, @@ -277,21 +288,8 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { test("does nothing when the category already exists") { for { _ <- ZIO.debug("test 2 started") - modRepo <- ZIO.service[GitModificationRepository] - techniqueParse <- ZIO.service[TechniqueParser] - techniqueCompiler <- ZIO.service[TechniqueCompiler] gitRepositoryProvider <- ZIO.service[GitRepositoryProviderImpl] - personIdentservice <- ZIO.service[TrivialPersonIdentService] - groupOwner <- ZIO.service[GroupOwner] - techniqueArchive = new TechniqueArchiverImpl( - gitRepo = gitRepositoryProvider, - xmlPrettyPrinter = prettyPrinter, - gitModificationRepository = modRepo, - personIdentservice = personIdentservice, - techniqueParser = techniqueParse, - techniqueCompiler = techniqueCompiler, - groupOwner = groupOwner.value - ) + techniqueArchive <- ZIO.service[TechniqueArchiverImpl] _ <- techniqueArchive .saveTechniqueCategory( catPath, @@ -312,7 +310,8 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { StubbedTechniqueCompiler.layer, gitRepositoryProviderLayer, ZLayer.succeed(new TrivialPersonIdentService()), - currentUserNameLayer + currentUserNameLayer, + techniqueArchiverLayer ) @@ TestAspect.sequential ) From ccea060e4cb2838d0db149a863638bc8b3658b4b Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 12:25:45 +0100 Subject: [PATCH 05/15] step 5: tests should be written against interfaces when possible --- .../services/JGitRepositoryTest2.scala | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala index 223bda85f7..690ade9569 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala @@ -60,14 +60,13 @@ import com.normation.rudder.ncf.TechniqueCompilationOutput import com.normation.rudder.ncf.TechniqueCompiler import com.normation.rudder.ncf.TechniqueCompilerApp import com.normation.rudder.repository.GitModificationRepository -import com.normation.rudder.repository.xml.RudderPrettyPrinter -import com.normation.rudder.repository.xml.TechniqueArchiverImpl -import com.normation.rudder.repository.xml.XmlArchiverUtils +import com.normation.rudder.repository.xml.{RudderPrettyPrinter, TechniqueArchiver, TechniqueArchiverImpl, XmlArchiverUtils} import com.normation.rudder.services.user.TrivialPersonIdentService import org.eclipse.jgit.lib.PersonIdent import org.eclipse.jgit.lib.Repository import org.eclipse.jgit.revwalk.RevWalk import org.eclipse.jgit.treewalk.TreeWalk + import scala.util.Random import zio.Chunk import zio.IO @@ -176,16 +175,16 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { case class GroupOwner(value: String) // for test, we use as a group owner whatever git root directory has - def currentUserName(repo: GitRepositoryProviderImpl): GroupOwner = GroupOwner(repo.rootDirectory.groupName) + def currentUserName(repo: GitRepositoryProvider): GroupOwner = GroupOwner(repo.rootDirectory.groupName) - val currentUserNameLayer: ZLayer[GitRepositoryProviderImpl, Nothing, GroupOwner] = ZLayer { + val currentUserNameLayer: ZLayer[GitRepositoryProvider, Nothing, GroupOwner] = ZLayer { for { - repo <- ZIO.service[GitRepositoryProviderImpl] + repo <- ZIO.service[GitRepositoryProvider] } yield currentUserName(repo) } val techniqueArchiverLayer: ZLayer[ - GroupOwner & TrivialPersonIdentService & GitRepositoryProviderImpl & TechniqueCompiler & TechniqueParser & GitModificationRepository, + GroupOwner & TrivialPersonIdentService & GitRepositoryProvider & TechniqueCompiler & TechniqueParser & GitModificationRepository, Nothing, TechniqueArchiverImpl ] = ZLayer { @@ -193,7 +192,7 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { modRepo <- ZIO.service[GitModificationRepository] techniqueParse <- ZIO.service[TechniqueParser] techniqueCompiler <- ZIO.service[TechniqueCompiler] - gitRepositoryProvider <- ZIO.service[GitRepositoryProviderImpl] + gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] personIdentservice <- ZIO.service[TrivialPersonIdentService] groupOwner <- ZIO.service[GroupOwner] techniqueArchive = new TechniqueArchiverImpl( @@ -231,7 +230,7 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { } yield name def makeArchive( - repository: GitRepositoryProviderImpl, + repository: GitRepositoryProvider, modRepo: GitModificationRepository ): GitConfigItemRepository with XmlArchiverUtils = { new GitConfigItemRepository with XmlArchiverUtils { @@ -247,7 +246,7 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { for { gitRoot <- ZIO.service[TempDir] modRepo <- ZIO.service[GitModificationRepository] - gitRepositoryProvider <- ZIO.service[GitRepositoryProviderImpl] + gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] archive = makeArchive(gitRepositoryProvider, modRepo) files <- ZIO.foreachPar(1 to 50)(i => add(i)(gitRoot, archive)).withParallelism(16) @@ -259,8 +258,8 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { test("create a new file and commit if the category does not exist") { for { _ <- ZIO.debug("test 1 started") - gitRepositoryProvider <- ZIO.service[GitRepositoryProviderImpl] - techniqueArchive <- ZIO.service[TechniqueArchiverImpl] + gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] + techniqueArchive <- ZIO.service[TechniqueArchiver] _ <- techniqueArchive .saveTechniqueCategory( catPath, @@ -288,8 +287,8 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { test("does nothing when the category already exists") { for { _ <- ZIO.debug("test 2 started") - gitRepositoryProvider <- ZIO.service[GitRepositoryProviderImpl] - techniqueArchive <- ZIO.service[TechniqueArchiverImpl] + gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] + techniqueArchive <- ZIO.service[TechniqueArchiver] _ <- techniqueArchive .saveTechniqueCategory( catPath, From cca2ee25c53760d557e38810e127074df8372207 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 12:31:30 +0100 Subject: [PATCH 06/15] step 6: sanity changes now make the tests running in parallel and without sharing any state --- .../normation/cfclerk/services/JGitRepositoryTest2.scala | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala index 690ade9569..a75380468c 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala @@ -257,7 +257,6 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { suite("save a category")( test("create a new file and commit if the category does not exist") { for { - _ <- ZIO.debug("test 1 started") gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] techniqueArchive <- ZIO.service[TechniqueArchiver] _ <- techniqueArchive @@ -286,7 +285,6 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { }, test("does nothing when the category already exists") { for { - _ <- ZIO.debug("test 2 started") gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] techniqueArchive <- ZIO.service[TechniqueArchiver] _ <- techniqueArchive @@ -297,12 +295,11 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { EventActor("test"), s"test: commit add category ${catPath.mkString("/")}" ) - _ <- techniqueArchive.saveTechniqueCategory(catPath, category, modId, EventActor("test"), s"test: commit again") lastCommitMsg = gitRepositoryProvider.git.log().setMaxCount(1).call().iterator().next().getFullMessage } yield assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) } - ).provideShared( + ).provide( TempDir.layer, StubGitModificationRepository.layer, techniqueParserLayer, @@ -311,7 +308,7 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { ZLayer.succeed(new TrivialPersonIdentService()), currentUserNameLayer, techniqueArchiverLayer - ) @@ TestAspect.sequential + ) ) } From 7f371e837f12a382a36a1013c3402a69932de224 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 12:36:45 +0100 Subject: [PATCH 07/15] step 7: XmlArchiverUtils is not part of the test, removing it --- .../services/JGitRepositoryTest2.scala | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala index a75380468c..af987ea7a6 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala @@ -60,13 +60,14 @@ import com.normation.rudder.ncf.TechniqueCompilationOutput import com.normation.rudder.ncf.TechniqueCompiler import com.normation.rudder.ncf.TechniqueCompilerApp import com.normation.rudder.repository.GitModificationRepository -import com.normation.rudder.repository.xml.{RudderPrettyPrinter, TechniqueArchiver, TechniqueArchiverImpl, XmlArchiverUtils} +import com.normation.rudder.repository.xml.RudderPrettyPrinter +import com.normation.rudder.repository.xml.TechniqueArchiver +import com.normation.rudder.repository.xml.TechniqueArchiverImpl import com.normation.rudder.services.user.TrivialPersonIdentService import org.eclipse.jgit.lib.PersonIdent import org.eclipse.jgit.lib.Repository import org.eclipse.jgit.revwalk.RevWalk import org.eclipse.jgit.treewalk.TreeWalk - import scala.util.Random import zio.Chunk import zio.IO @@ -221,7 +222,7 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { } } - def add(i: Int)(gitRoot: TempDir, archive: GitConfigItemRepository with XmlArchiverUtils) = for { + def add(i: Int)(gitRoot: TempDir, archive: GitConfigItemRepository) = for { name <- getName(8).map(s => i.toString + "_" + s) file = gitRoot.path / name _ <- IOResult.attempt(file.write("something in " + name)) @@ -232,15 +233,10 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { def makeArchive( repository: GitRepositoryProvider, modRepo: GitModificationRepository - ): GitConfigItemRepository with XmlArchiverUtils = { - new GitConfigItemRepository with XmlArchiverUtils { - override val gitRepo: GitRepositoryProvider = repository - override def relativePath: String = "" - override def xmlPrettyPrinter = prettyPrinter - override def encoding: String = "UTF-8" - override def gitModificationRepository: GitModificationRepository = modRepo - override def groupOwner: String = currentUserName(repository).value - } + ): GitConfigItemRepository = new GitConfigItemRepository { + override val gitRepo: GitRepositoryProvider = repository + override def relativePath: String = "" + override def gitModificationRepository: GitModificationRepository = modRepo } for { From 0deded8b3e2e943f139100b8175804fcd84d1f94 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 12:48:21 +0100 Subject: [PATCH 08/15] step 8: first suite is actually about GitConfigItemRepository, make it more obvious --- .../services/JGitRepositoryTest2.scala | 79 +++++++++---------- 1 file changed, 36 insertions(+), 43 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala index af987ea7a6..07cb8de2fc 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala @@ -44,7 +44,6 @@ import com.normation.cfclerk.xmlparsers.SectionSpecParser import com.normation.cfclerk.xmlparsers.TechniqueParser import com.normation.cfclerk.xmlparsers.VariableSpecParser import com.normation.errors -import com.normation.errors.Inconsistency import com.normation.errors.IOResult import com.normation.errors.RudderError import com.normation.errors.effectUioUnit @@ -68,9 +67,7 @@ import org.eclipse.jgit.lib.PersonIdent import org.eclipse.jgit.lib.Repository import org.eclipse.jgit.revwalk.RevWalk import org.eclipse.jgit.treewalk.TreeWalk -import scala.util.Random import zio.Chunk -import zio.IO import zio.System import zio.ULayer import zio.ZIO @@ -208,50 +205,46 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { } yield techniqueArchive } - val spec: Spec[Any, RudderError] = suite("The test lib")( - // to assess the usefulness of semaphore, you can remove `gitRepo.semaphore.withPermit` - // in `commitAddFile` to check that you get the JGitInternalException. - // More advanced tests may be needed to handle more complex cases of concurrent access, - // see: https://issues.rudder.io/issues/19910 - test("not throw JGitInternalError on concurrent write") { - - def getName(length: Int): IO[errors.RudderError, String] = { - if (length < 1) Inconsistency("Length must be positive").fail - else { - IOResult.attempt("")(Random.alphanumeric.take(length).toList.mkString("")) - } - } - - def add(i: Int)(gitRoot: TempDir, archive: GitConfigItemRepository) = for { - name <- getName(8).map(s => i.toString + "_" + s) - file = gitRoot.path / name - _ <- IOResult.attempt(file.write("something in " + name)) - actor = new PersonIdent("test", "test@test.com") - _ <- archive.commitAddFileWithModId(ModificationId(name), actor, name, "add " + name) - } yield name - - def makeArchive( - repository: GitRepositoryProvider, - modRepo: GitModificationRepository - ): GitConfigItemRepository = new GitConfigItemRepository { + val gitConfigItemRepositoryLayer + : ZLayer[GitModificationRepository & GitRepositoryProvider, Nothing, GitConfigItemRepository] = { + ZLayer { + for { + repository <- ZIO.service[GitRepositoryProvider] + modRepo <- ZIO.service[GitModificationRepository] + } yield new GitConfigItemRepository { override val gitRepo: GitRepositoryProvider = repository override def relativePath: String = "" override def gitModificationRepository: GitModificationRepository = modRepo } + } + } - for { - gitRoot <- ZIO.service[TempDir] - modRepo <- ZIO.service[GitModificationRepository] - gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] + val spec: Spec[Any, RudderError] = suite("The test lib")( + suite("GitConfigItemRepository")( + // to assess the usefulness of semaphore, you can remove `gitRepo.semaphore.withPermit` + // in `commitAddFile` to check that you get the JGitInternalException. + // More advanced tests may be needed to handle more complex cases of concurrent access, + // see: https://issues.rudder.io/issues/19910 + test("should not throw JGitInternalError on concurrent write") { + + def addFileToRepositoryAndCommit(i: Int)(gitRoot: TempDir, archive: GitConfigItemRepository) = for { + name <- zio.Random.nextString(8).map(s => i.toString + "_" + s) + file = gitRoot.path / name + _ <- IOResult.attempt(file.write("something in " + name)) + actor = new PersonIdent("test", "test@test.com") + _ <- archive.commitAddFileWithModId(ModificationId(name), actor, name, "add " + name) + } yield name - archive = makeArchive(gitRepositoryProvider, modRepo) - files <- ZIO.foreachPar(1 to 50)(i => add(i)(gitRoot, archive)).withParallelism(16) - created <- readElementsAt(gitRepositoryProvider.db, "refs/heads/master") - } yield assert(created)(hasSameElements(files)) + for { + gitRoot <- ZIO.service[TempDir] + gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] + archive <- ZIO.service[GitConfigItemRepository] + files <- ZIO.foreachPar(1 to 50)(i => addFileToRepositoryAndCommit(i)(gitRoot, archive)).withParallelism(16) + created <- readElementsAt(gitRepositoryProvider.db, "refs/heads/master") + } yield assert(created)(hasSameElements(files)) - }.provide(TempDir.layer, StubGitModificationRepository.layer, gitRepositoryProviderLayer), - suite("save a category")( - test("create a new file and commit if the category does not exist") { + }.provide(TempDir.layer, StubGitModificationRepository.layer, gitRepositoryProviderLayer, gitConfigItemRepositoryLayer), + suite("save a category")(test("create a new file and commit if the category does not exist") { for { gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] techniqueArchive <- ZIO.service[TechniqueArchiver] @@ -278,7 +271,7 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { ) && assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) } - }, + }), test("does nothing when the category already exists") { for { gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] @@ -291,8 +284,8 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { EventActor("test"), s"test: commit add category ${catPath.mkString("/")}" ) - _ <- techniqueArchive.saveTechniqueCategory(catPath, category, modId, EventActor("test"), s"test: commit again") - lastCommitMsg = gitRepositoryProvider.git.log().setMaxCount(1).call().iterator().next().getFullMessage + _ <- techniqueArchive.saveTechniqueCategory(catPath, category, modId, EventActor("test"), s"test: commit again") + lastCommitMsg = gitRepositoryProvider.git.log().setMaxCount(1).call().iterator().next().getFullMessage } yield assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) } ).provide( From 3bbff63f511a7c7ce8189767660677c6a3c45e09 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 12:51:06 +0100 Subject: [PATCH 09/15] step 9: second suite is actually about TechniqueArchiver.saveTechniqueCategory --- .../cfclerk/services/JGitRepositoryTest2.scala | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala index 07cb8de2fc..a4766e8cc0 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala @@ -243,8 +243,10 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { created <- readElementsAt(gitRepositoryProvider.db, "refs/heads/master") } yield assert(created)(hasSameElements(files)) - }.provide(TempDir.layer, StubGitModificationRepository.layer, gitRepositoryProviderLayer, gitConfigItemRepositoryLayer), - suite("save a category")(test("create a new file and commit if the category does not exist") { + }.provide(TempDir.layer, StubGitModificationRepository.layer, gitRepositoryProviderLayer, gitConfigItemRepositoryLayer) + ), + suite("TechniqueArchiver.saveTechniqueCategory")( + test("should create a new file and commit if the category does not exist") { for { gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] techniqueArchive <- ZIO.service[TechniqueArchiver] @@ -271,8 +273,8 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { ) && assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) } - }), - test("does nothing when the category already exists") { + }, + test("should do nothing when the category already exists") { for { gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] techniqueArchive <- ZIO.service[TechniqueArchiver] From 9d16adbfdab9dffcd0f821805cd7bad2c37bae94 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 14:12:22 +0100 Subject: [PATCH 10/15] step 10: delete original JGitRepositoryTest --- .../cfclerk/services/JGitRepositoryTest.scala | 254 ------------------ 1 file changed, 254 deletions(-) delete mode 100644 webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala deleted file mode 100644 index 5dd366fc6d..0000000000 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala +++ /dev/null @@ -1,254 +0,0 @@ -/* - ************************************************************************************* - * Copyright 2021 Normation SAS - ************************************************************************************* - * - * This file is part of Rudder. - * - * Rudder is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * In accordance with the terms of section 7 (7. Additional Terms.) of - * the GNU General Public License version 3, the copyright holders add - * the following Additional permissions: - * Notwithstanding to the terms of section 5 (5. Conveying Modified Source - * Versions) and 6 (6. Conveying Non-Source Forms.) of the GNU General - * Public License version 3, when you create a Related Module, this - * Related Module is not considered as a part of the work and may be - * distributed under the license agreement of your choice. - * A "Related Module" means a set of sources files including their - * documentation that, without modification of the Source Code, enables - * supplementary functions or services in addition to those offered by - * the Software. - * - * Rudder is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with Rudder. If not, see . - - * - ************************************************************************************* - */ - -package com.normation.cfclerk.services - -import better.files.File -import com.normation.cfclerk.domain.TechniqueCategoryMetadata -import com.normation.cfclerk.services.impl.SystemVariableSpecServiceImpl -import com.normation.cfclerk.xmlparsers.SectionSpecParser -import com.normation.cfclerk.xmlparsers.TechniqueParser -import com.normation.cfclerk.xmlparsers.VariableSpecParser -import com.normation.errors -import com.normation.errors.Inconsistency -import com.normation.errors.IOResult -import com.normation.errors.effectUioUnit -import com.normation.eventlog.EventActor -import com.normation.eventlog.ModificationId -import com.normation.rudder.db.DB -import com.normation.rudder.git.GitCommitId -import com.normation.rudder.git.GitConfigItemRepository -import com.normation.rudder.git.GitRepositoryProvider -import com.normation.rudder.git.GitRepositoryProviderImpl -import com.normation.rudder.ncf.EditorTechnique -import com.normation.rudder.ncf.TechniqueCompilationOutput -import com.normation.rudder.ncf.TechniqueCompiler -import com.normation.rudder.ncf.TechniqueCompilerApp -import com.normation.rudder.repository.GitModificationRepository -import com.normation.rudder.repository.xml.RudderPrettyPrinter -import com.normation.rudder.repository.xml.TechniqueArchiverImpl -import com.normation.rudder.repository.xml.XmlArchiverUtils -import com.normation.rudder.services.user.TrivialPersonIdentService -import com.normation.zio.* -import net.liftweb.common.Loggable -import org.apache.commons.io.FileUtils -import org.eclipse.jgit.lib.PersonIdent -import org.eclipse.jgit.lib.Repository -import org.eclipse.jgit.revwalk.RevWalk -import org.joda.time.DateTime -import org.junit.runner.RunWith -import org.specs2.mutable.Specification -import org.specs2.runner.JUnitRunner -import org.specs2.specification.AfterAll -import scala.annotation.nowarn -import scala.util.Random -import zio.{System as _, *} -import zio.syntax.* - -/** - * Details of tests executed in each instances of - * the test. - * To see values for gitRoot, ptLib, etc, see at the end - * of that file. - */ -@nowarn("msg=a type was inferred to be `\\w+`; this may indicate a programming error.") -@RunWith(classOf[JUnitRunner]) -class JGitRepositoryTest extends Specification with Loggable with AfterAll { - - val gitRoot: File = File("/tmp/test-jgit-" + DateTime.now().toString()) - - // Set sequential execution - sequential - - /** - * Add a switch to be able to see tmp files (not clean temps) with - * -Dtests.clean.tmp=false - */ - override def afterAll(): Unit = { - if (java.lang.System.getProperty("tests.clean.tmp") != "false") { - logger.info("Deleting directory " + gitRoot.pathAsString) - FileUtils.deleteDirectory(gitRoot.toJava) - } - } - - gitRoot.createDirectories() - - val repo: GitRepositoryProviderImpl = GitRepositoryProviderImpl.make(gitRoot.pathAsString).runNow - val prettyPrinter: RudderPrettyPrinter = new RudderPrettyPrinter(Int.MaxValue, 2) - val modRepo: GitModificationRepository = new GitModificationRepository { - override def getCommits(modificationId: ModificationId): IOResult[Option[GitCommitId]] = None.succeed - override def addCommit(commit: GitCommitId, modId: ModificationId): IOResult[DB.GitCommitJoin] = - DB.GitCommitJoin(commit, modId).succeed - } - val personIdent: TrivialPersonIdentService = new TrivialPersonIdentService() - val techniqueParser: TechniqueParser = { - val varParser = new VariableSpecParser - new TechniqueParser(varParser, new SectionSpecParser(varParser), new SystemVariableSpecServiceImpl()) - } - val techniqueCompiler = new TechniqueCompiler { - override def compileTechnique(technique: EditorTechnique): IOResult[TechniqueCompilationOutput] = { - TechniqueCompilationOutput(TechniqueCompilerApp.Rudderc, 0, Chunk.empty, "", "", "").succeed - } - - override def getCompilationOutputFile(technique: EditorTechnique): File = File("compilation-config.yml") - - override def getCompilationConfigFile(technique: EditorTechnique): File = File("compilation-output.yml") - } - - // for test, we use as a group owner whatever git root directory has - val currentUserName: String = repo.rootDirectory.groupName - - val archive: GitConfigItemRepository with XmlArchiverUtils = new GitConfigItemRepository with XmlArchiverUtils { - override val gitRepo: GitRepositoryProvider = repo - override def relativePath: String = "" - override def xmlPrettyPrinter = prettyPrinter - override def encoding: String = "UTF-8" - override def gitModificationRepository: GitModificationRepository = modRepo - - override def groupOwner: String = currentUserName - } - - val techniqueArchive: TechniqueArchiverImpl = - new TechniqueArchiverImpl(repo, prettyPrinter, modRepo, personIdent, techniqueParser, techniqueCompiler, currentUserName) - - // listing files at a commit is complicated - - import org.eclipse.jgit.treewalk.TreeWalk - - def readElementsAt(repository: Repository, commit: String): ZIO[Any, errors.SystemError, List[String]] = { - val ref = repository.findRef(commit) - - // a RevWalk allows to walk over commits based on some filtering that is defined - val walkM = ZIO.acquireRelease(IOResult.attempt(new RevWalk(repository)))(x => effectUioUnit(x.close())) - val treeWalkM = ZIO.acquireRelease(IOResult.attempt(new TreeWalk(repository)))(x => effectUioUnit(x.close())) - - ZIO.scoped[Any]( - walkM.flatMap(walk => { - for { - commit <- IOResult.attempt(walk.parseCommit(ref.getObjectId)) - tree <- IOResult.attempt(commit.getTree) - res <- ZIO.scoped(treeWalkM.flatMap { treeWalk => - treeWalk.setRecursive(true) // ok, can't throw exception - - IOResult.attempt(treeWalk.addTree(tree)) *> - ZIO.loop(treeWalk)(_.next, identity)(x => IOResult.attempt(x.getPathString)) - }) - } yield res - }) - ) - } - - "The test lib" should { - "not throw JGitInternalError on concurrent write" in { - - // to assess the usefulness of semaphore, you can remove `gitRepo.semaphore.withPermit` - // in `commitAddFile` to check that you get the JGitInternalException. - // More advanced tests may be needed to handle more complex cases of concurrent access, - // see: https://issues.rudder.io/issues/19910 - - val actor = new PersonIdent("test", "test@test.com") - - def getName(length: Int) = { - if (length < 1) Inconsistency("Length must be positive").fail - else { - IOResult.attempt("")(Random.alphanumeric.take(length).toList.mkString("")) - } - } - def add(i: Int) = (for { - name <- getName(8).map(s => i.toString + "_" + s) - file = gitRoot / name - f <- IOResult.attempt(file.write("something in " + name)) - _ <- archive.commitAddFileWithModId(ModificationId(name), actor, name, "add " + name) - } yield (name)) - - logger.debug(s"Commiting files in: " + gitRoot.pathAsString) - val files = ZIO.foreachPar(1 to 50)(i => add(i)).withParallelism(16).runNow - - val created = readElementsAt(repo.db, "refs/heads/master").runNow - created must containTheSameElementsAs(files) - - } - - "save a category" should { - - val category = TechniqueCategoryMetadata("My new category", "A new category", isSystem = false) - val catPath = List("systemSettings", "myNewCategory") - - val modId = new ModificationId("add-technique-cat") - - "create a new file and commit if the category does not exist" in { - - techniqueArchive - .saveTechniqueCategory( - catPath, - category, - modId, - EventActor("test"), - s"test: commit add category ${catPath.mkString("/")}" - ) - .runNow - - val catFile = repo.rootDirectory / "techniques" / "systemSettings" / "myNewCategory" / "category.xml" - - val xml = catFile.contentAsString - - val lastCommitMsg = repo.git.log().setMaxCount(1).call().iterator().next().getFullMessage - - // note: no false ; it's only written when true - (xml === - """ - | My new category - | A new category - |""".stripMargin) and ( - lastCommitMsg === "test: commit add category systemSettings/myNewCategory" - ) - - } - - "does nothing when the category already exsits" in { - techniqueArchive.saveTechniqueCategory(catPath, category, modId, EventActor("test"), s"test: commit again").runNow - val lastCommitMsg = repo.git.log().setMaxCount(1).call().iterator().next().getFullMessage - - // last commit must be the old one - lastCommitMsg === "test: commit add category systemSettings/myNewCategory" - - } - } - - } - -} From 5093744544127782b50c889a473e93becf4f4f27 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 14:19:44 +0100 Subject: [PATCH 11/15] step 11: turn GitItemRepository into a class This ensures that it's closed to modification, that way testing this class is enough to test its features, you don't need to test features in subclasses anymore. --- .../rudder/facts/nodes/NodeFactStorage.scala | 29 +- .../rudder/git/GitItemRepository.scala | 42 +-- .../rudder/ncf/EditorTechniqueReader.scala | 40 ++- .../rudder/repository/xml/GitArchivers.scala | 314 ++++++++++-------- .../category/GitRuleCategoryArchiver.scala | 26 +- ...ryTest2.scala => JGitRepositoryTest.scala} | 26 +- .../bootstrap/liftweb/RudderConfig.scala | 1 - .../TestMigrateJsonTechniquesToYaml.scala | 27 +- 8 files changed, 267 insertions(+), 238 deletions(-) rename webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/{JGitRepositoryTest2.scala => JGitRepositoryTest.scala} (92%) diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/facts/nodes/NodeFactStorage.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/facts/nodes/NodeFactStorage.scala index b7cefb777b..97914e46a8 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/facts/nodes/NodeFactStorage.scala +++ b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/facts/nodes/NodeFactStorage.scala @@ -355,12 +355,15 @@ object GitNodeFactStorageImpl { * a special operation) */ class GitNodeFactStorageImpl( - override val gitRepo: GitRepositoryProvider, - groupOwner: Option[String], - actuallyCommit: Boolean -) extends NodeFactStorage with GitItemRepository with SerializeFacts[(NodeId, InventoryStatus), NodeFact] { + gitRepo: GitRepositoryProvider, + groupOwner: Option[String], + actuallyCommit: Boolean +) extends NodeFactStorage with SerializeFacts[(NodeId, InventoryStatus), NodeFact] { + + private val relativePath = "nodes" + + private val gitItemRepository: GitItemRepository = new GitItemRepository(gitRepo, relativePath) - override val relativePath = "nodes" override val entity: String = "node" override val fileFormat: String = "10" val committer = new PersonIdent("rudder-fact", "email not set") @@ -489,9 +492,9 @@ class GitNodeFactStorageImpl( case Some(go) => IOResult.attempt(file.setGroup(go)) } _ <- ZIO.when(actuallyCommit) { - commitAddFile( + gitItemRepository.commitAddFile( committer, - toGitPath(file.toJava), + gitItemRepository.toGitPath(file.toJava), s"Save inventory facts for ${merged.rudderSettings.status.name} node '${merged.fqdn}' (${merged.id.value})" ) } @@ -519,7 +522,11 @@ class GitNodeFactStorageImpl( } def delete(file: File) = { (if (actuallyCommit) { - commitRmFile(committer, toGitPath(file.toJava), s"Updating facts for node '${nodeId.value}': deleted") + gitItemRepository.commitRmFile( + committer, + gitItemRepository.toGitPath(file.toJava), + s"Updating facts for node '${nodeId.value}': deleted" + ) } else { IOResult.attempt(file.delete()) }).flatMap(_ => fileToNode(file)(attrs).map(Some(_)).catchAll(_ => None.succeed)).map { @@ -553,10 +560,10 @@ class GitNodeFactStorageImpl( // we need to overwrite IOResult.attempt(fromFile.moveTo(toFile)(File.CopyOptions(overwrite = true))) *> ZIO.when(actuallyCommit) { - commitMvDirectory( + gitItemRepository.commitMvDirectory( committer, - toGitPath(fromFile.toJava), - toGitPath(toFile.toJava), + gitItemRepository.toGitPath(fromFile.toJava), + gitItemRepository.toGitPath(toFile.toJava), s"Updating facts for node '${nodeId.value}' to status: ${to.name}" ) } *> fileToNode(toFile)(SelectFacts.none).map(Some(_)).catchAll(_ => None.succeed).map { diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala index cdeea12586..69dee0ffb3 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala +++ b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala @@ -56,17 +56,17 @@ import zio.* /** * Utility trait that factor out file commits. */ -trait GitItemRepository { +class GitItemRepository( + // the underlying git repository where items are saved + gitRepo: GitRepositoryProvider, + // relative path from git root directory where items for that implementation + // are saved. For example, rules are saved in `/var/rudder/config-repo/rules`, + // where `/var/rudder/config-repos` is the root directory provided by gitRepos, + // the relativePath is `rules` + relativePath: String +) { - // the underlying git repository where items are saved - def gitRepo: GitRepositoryProvider - // relative path from git root directory where items for that implementation - // are saved. For example, rules are saved in `/var/rudder/config-repo/rules`, - // where `/var/rudder/config-repos` is the root directory provided by gitRepos, - // the relativePath is `rules` - def relativePath: String - - lazy val getItemDirectory: File = { + final lazy val getItemDirectory: File = { /** * Create directory given in argument if does not exist, checking @@ -109,13 +109,13 @@ trait GitItemRepository { // better.files.File.pathAsString is normalized without an ending slash, an Git path are relative to "/" // *without* the leading slash. - def toGitPath(fsPath: File): String = fsPath.getPath.replace(gitRepo.rootDirectory.pathAsString + "/", "") + final def toGitPath(fsPath: File): String = fsPath.getPath.replace(gitRepo.rootDirectory.pathAsString + "/", "") /** * Files in gitPath are added. * commitMessage is used for the message of the commit. */ - def commitAddFile(commiter: PersonIdent, gitPath: String, commitMessage: String): IOResult[GitCommitId] = { + final def commitAddFile(commiter: PersonIdent, gitPath: String, commitMessage: String): IOResult[GitCommitId] = { gitRepo.semaphore.withPermit( for { _ <- GitArchiveLoggerPure.debug(s"Add file '${gitPath}' from configuration repository") @@ -140,7 +140,7 @@ trait GitItemRepository { * Files in gitPath are removed. * commitMessage is used for the message of the commit. */ - def commitRmFile(commiter: PersonIdent, gitPath: String, commitMessage: String): IOResult[GitCommitId] = { + final def commitRmFile(commiter: PersonIdent, gitPath: String, commitMessage: String): IOResult[GitCommitId] = { gitRepo.semaphore.withPermit( for { _ <- GitArchiveLoggerPure.debug(s"remove file '${gitPath}' from configuration repository") @@ -167,7 +167,7 @@ trait GitItemRepository { * 'gitRepo.git added' (with and without the 'update' mode). * commitMessage is used for the message of the commit. */ - def commitMvDirectory( + final def commitMvDirectory( commiter: PersonIdent, oldGitPath: String, newGitPath: String, @@ -202,15 +202,17 @@ trait GitItemRepository { * An extension of simple GitItemRepositoty that in addition knows how to link commitId and modId together. * Used for all configuration objects, but not for facts. */ -trait GitConfigItemRepository extends GitItemRepository { - - def gitModificationRepository: GitModificationRepository +class GitConfigItemRepository( + gitRepo: GitRepositoryProvider, + relativePath: String, + gitModificationRepository: GitModificationRepository +) extends GitItemRepository(gitRepo, relativePath) { /** * Files in gitPath are added. * commitMessage is used for the message of the commit. */ - def commitAddFileWithModId( + final def commitAddFileWithModId( modId: ModificationId, commiter: PersonIdent, gitPath: String, @@ -228,7 +230,7 @@ trait GitConfigItemRepository extends GitItemRepository { * Files in gitPath are removed. * commitMessage is used for the message of the commit. */ - def commitRmFileWithModId( + final def commitRmFileWithModId( modId: ModificationId, commiter: PersonIdent, gitPath: String, @@ -249,7 +251,7 @@ trait GitConfigItemRepository extends GitItemRepository { * 'gitRepo.git added' (with and without the 'update' mode). * commitMessage is used for the message of the commit. */ - def commitMvDirectoryWithModId( + final def commitMvDirectoryWithModId( modId: ModificationId, commiter: PersonIdent, oldGitPath: String, diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/ncf/EditorTechniqueReader.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/ncf/EditorTechniqueReader.scala index 3add113f95..142e8c8661 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/ncf/EditorTechniqueReader.scala +++ b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/ncf/EditorTechniqueReader.scala @@ -38,20 +38,25 @@ trait EditorTechniqueReader { } class EditorTechniqueReaderImpl( - uuidGen: StringUuidGenerator, - personIdentService: PersonIdentService, - override val gitRepo: GitRepositoryProvider, - override val xmlPrettyPrinter: RudderPrettyPrinter, - override val gitModificationRepository: GitModificationRepository, - override val encoding: String, - override val groupOwner: String, - yamlTechniqueSerializer: YamlTechniqueSerializer, - parameterTypeService: ParameterTypeService, - ruddercCmd: String, - methodsSystemLib: String, - methodsLocalLib: String -) extends EditorTechniqueReader with GitConfigItemRepository with XmlArchiverUtils { - override val relativePath: String = "ncf" + uuidGen: StringUuidGenerator, + personIdentService: PersonIdentService, + gitRepo: GitRepositoryProvider, + override val xmlPrettyPrinter: RudderPrettyPrinter, + gitModificationRepository: GitModificationRepository, + override val encoding: String, + override val groupOwner: String, + yamlTechniqueSerializer: YamlTechniqueSerializer, + parameterTypeService: ParameterTypeService, + ruddercCmd: String, + methodsSystemLib: String, + methodsLocalLib: String +) extends EditorTechniqueReader with XmlArchiverUtils { + + private val relativePath: String = "ncf" + + private val gitConfigItemRepository: GitConfigItemRepository = + new GitConfigItemRepository(gitRepo, relativePath, gitModificationRepository) + val configuration_repository = gitRepo.rootDirectory val ncfRootDir: File = configuration_repository / relativePath val methodsFile: File = ncfRootDir / "generic_methods.json" @@ -165,7 +170,12 @@ class EditorTechniqueReaderImpl( // commit file modId = ModificationId(uuidGen.newUuid) ident <- personIdentService.getPersonIdentOrDefault(RudderEventActor.name) - _ <- commitAddFileWithModId(modId, ident, toGitPath(methodsFile.toJava), "Saving updated generic methods definition") + _ <- gitConfigItemRepository.commitAddFileWithModId( + modId = modId, + commiter = ident, + gitPath = gitConfigItemRepository.toGitPath(methodsFile.toJava), + commitMessage = "Saving updated generic methods definition" + ) } yield { res } diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/xml/GitArchivers.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/xml/GitArchivers.scala index 901eee79ca..2f19446763 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/xml/GitArchivers.scala +++ b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/xml/GitArchivers.scala @@ -83,27 +83,32 @@ import zio.* import zio.syntax.* class GitRuleArchiverImpl( - override val gitRepo: GitRepositoryProvider, - ruleSerialisation: RuleSerialisation, - ruleRootDir: String, // relative path ! - + override val gitRepo: GitRepositoryProvider, + ruleSerialisation: RuleSerialisation, + ruleRootDir: String, // relative path ! override val xmlPrettyPrinter: RudderPrettyPrinter, override val gitModificationRepository: GitModificationRepository, override val encoding: String, override val groupOwner: String -) extends GitRuleArchiver with XmlArchiverUtils with NamedZioLogger with GitConfigItemRepository with GitArchiverFullCommitUtils - with BuildFilePaths[RuleId] { +) extends GitRuleArchiver with XmlArchiverUtils with NamedZioLogger with GitArchiverFullCommitUtils with BuildFilePaths[RuleId] { - override def loggerName: String = this.getClass.getName override val relativePath = ruleRootDir - override val tagPrefix = "archives/configurations-rules/" + + private val gitConfigItemRepository: GitConfigItemRepository = + new GitConfigItemRepository(gitRepo, relativePath, gitModificationRepository) + + override def getItemDirectory: File = gitConfigItemRepository.getItemDirectory + + override def loggerName: String = this.getClass.getName + + override val tagPrefix = "archives/configurations-rules/" override def getFileName(ruleId: RuleId): String = ruleId.serialize + ".xml" def archiveRule(rule: Rule, doCommit: Option[(ModificationId, PersonIdent, Option[String])]): IOResult[GitPath] = { for { crFile <- newFile(rule.id).toIO - gitPath = toGitPath(crFile) + gitPath = gitConfigItemRepository.toGitPath(crFile) archive <- writeXml( crFile, @@ -112,7 +117,12 @@ class GitRuleArchiverImpl( ) commit <- doCommit match { case Some((modId, commiter, reason)) => - commitAddFileWithModId(modId, commiter, gitPath, s"Archive rule with ID '${rule.id.serialize}'${GET(reason)}") + gitConfigItemRepository.commitAddFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = s"Archive rule with ID '${rule.id.serialize}'${GET(reason)}" + ) case None => ZIO.unit } } yield { @@ -133,18 +143,18 @@ class GitRuleArchiverImpl( def deleteRule(ruleId: RuleId, doCommit: Option[(ModificationId, PersonIdent, Option[String])]): IOResult[GitPath] = { for { crFile <- newFile(ruleId).toIO - gitPath = toGitPath(crFile) + gitPath = gitConfigItemRepository.toGitPath(crFile) _ <- ZIO.whenZIO(IOResult.attempt(crFile.exists)) { for { deleted <- IOResult.attempt(FileUtils.forceDelete(crFile)) _ <- logPure.debug("Deleted archive of rule: " + crFile.getPath) _ <- doCommit match { case Some((modId, commiter, reason)) => - commitRmFileWithModId( - modId, - commiter, - gitPath, - s"Delete archive of rule with ID '${ruleId.serialize}'${GET(reason)}" + gitConfigItemRepository.commitRmFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = s"Delete archive of rule with ID '${ruleId.serialize}'${GET(reason)}" ) case None => ZIO.unit } @@ -309,19 +319,18 @@ object TechniqueFiles { * - to migrate from `technique.json` to `technique.yml` if needed */ class TechniqueArchiverImpl( - override val gitRepo: GitRepositoryProvider, - override val xmlPrettyPrinter: RudderPrettyPrinter, - override val gitModificationRepository: GitModificationRepository, - personIdentservice: PersonIdentService, - techniqueParser: TechniqueParser, - techniqueCompiler: TechniqueCompiler, - override val groupOwner: String -) extends GitConfigItemRepository with XmlArchiverUtils with TechniqueArchiver { - - override val encoding: String = "UTF-8" + val gitRepo: GitRepositoryProvider, + override val xmlPrettyPrinter: RudderPrettyPrinter, + personIdentservice: PersonIdentService, + techniqueParser: TechniqueParser, + techniqueCompiler: TechniqueCompiler, + override val groupOwner: String +) extends XmlArchiverUtils with TechniqueArchiver { // we can't use "techniques" for relative path because of ncf and dsc files. This is an architecture smell, we need to clean it. - override val relativePath = "techniques" + val relativePath = "techniques" + + override val encoding: String = "UTF-8" def deleteTechnique( techniqueId: TechniqueId, @@ -484,11 +493,17 @@ class GitActiveTechniqueCategoryArchiverImpl( override val encoding: String, serializedCategoryName: String, override val groupOwner: String -) extends GitActiveTechniqueCategoryArchiver with Loggable with GitConfigItemRepository with XmlArchiverUtils +) extends GitActiveTechniqueCategoryArchiver with Loggable with XmlArchiverUtils with BuildCategoryPathName[ActiveTechniqueCategoryId] with GitArchiverFullCommitUtils { - override def loggerName: String = this.getClass.getName override lazy val relativePath = techniqueLibraryRootDir + + private val gitConfigItemRepository: GitConfigItemRepository = + new GitConfigItemRepository(gitRepo, relativePath, gitModificationRepository) + + override def getItemDirectory: File = gitConfigItemRepository.getItemDirectory + + override def loggerName: String = this.getClass.getName override def getCategoryName(categoryId: ActiveTechniqueCategoryId) = categoryId.value override lazy val tagPrefix = "archives/directives/" @@ -506,7 +521,7 @@ class GitActiveTechniqueCategoryArchiverImpl( for { uptcFile <- newActiveTechniquecFile(uptc.id, newParents) - gitPath = toGitPath(uptcFile) + gitPath = gitConfigItemRepository.toGitPath(uptcFile) archive <- writeXml( uptcFile, activeTechniqueCategorySerialisation.serialise(uptc), @@ -518,16 +533,16 @@ class GitActiveTechniqueCategoryArchiverImpl( oldParents match { case Some(olds) => newActiveTechniquecFile(uptc.id, olds).flatMap(oldUptcFile => { - commitMvDirectoryWithModId( + gitConfigItemRepository.commitMvDirectoryWithModId( modId, commiter, - toGitPath(oldUptcFile), + gitConfigItemRepository.toGitPath(oldUptcFile), uptcGitPath, "Move archive of technique library category with ID '%s'%s".format(uptc.id.value, GET(reason)) ) }) case None => - commitAddFileWithModId( + gitConfigItemRepository.commitAddFileWithModId( modId, commiter, uptcGitPath, @@ -556,7 +571,7 @@ class GitActiveTechniqueCategoryArchiverImpl( ): IOResult[GitPath] = { for { uptcFile <- newActiveTechniquecFile(uptcId, getParents) - gitPath = toGitPath(uptcFile) + gitPath = gitConfigItemRepository.toGitPath(uptcFile) // don't forget to delete the category *directory* _ <- ZIO.whenZIO(IOResult.attempt(uptcFile.exists)) { for { @@ -564,11 +579,11 @@ class GitActiveTechniqueCategoryArchiverImpl( _ <- logPure.debug("Deleted archived technique library category: " + uptcFile.getPath) _ <- gitCommit match { case Some((modId, commiter, reason)) => - commitRmFileWithModId( - modId, - commiter, - gitPath, - s"Delete archive of technique library category with ID '${uptcId.value}'${GET(reason)}" + gitConfigItemRepository.commitRmFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = s"Delete archive of technique library category with ID '${uptcId.value}'${GET(reason)}" ) case None => ZIO.unit } @@ -726,21 +741,27 @@ class UpdatePiOnActiveTechniqueEvent( * A specific trait to create archive of an active technique. */ class GitActiveTechniqueArchiverImpl( - override val gitRepo: GitRepositoryProvider, + gitRepo: GitRepositoryProvider, activeTechniqueSerialisation: ActiveTechniqueSerialisation, techniqueLibraryRootDir: String, // relative path ! - override val xmlPrettyPrinter: RudderPrettyPrinter, - override val gitModificationRepository: GitModificationRepository, - val uptModificationCallback: Buffer[ActiveTechniqueModificationCallback], - override val encoding: String, - val activeTechniqueFileName: String, - override val groupOwner: String -) extends GitActiveTechniqueArchiver with NamedZioLogger with GitConfigItemRepository with XmlArchiverUtils + override val xmlPrettyPrinter: RudderPrettyPrinter, + val gitModificationRepository: GitModificationRepository, + val uptModificationCallback: Buffer[ActiveTechniqueModificationCallback], + override val encoding: String, + val activeTechniqueFileName: String, + override val groupOwner: String +) extends GitActiveTechniqueArchiver with NamedZioLogger with XmlArchiverUtils with BuildCategoryPathName[ActiveTechniqueCategoryId] { + private lazy val relativePath = techniqueLibraryRootDir + + private val gitConfigItemRepository: GitConfigItemRepository = + new GitConfigItemRepository(gitRepo, relativePath, gitModificationRepository) + + override def getItemDirectory: File = gitConfigItemRepository.getItemDirectory + override def loggerName: String = this.getClass.getName - override lazy val relativePath = techniqueLibraryRootDir override def getCategoryName(categoryId: ActiveTechniqueCategoryId) = categoryId.value private def newActiveTechniqueFile(ptName: TechniqueName, parents: List[ActiveTechniqueCategoryId]) = { @@ -763,7 +784,7 @@ class GitActiveTechniqueArchiverImpl( ): IOResult[(GitPath, Seq[DirectiveNotArchived])] = { for { uptFile <- newActiveTechniqueFile(activeTechnique.techniqueName, parents) - gitPath = toGitPath(uptFile) + gitPath = gitConfigItemRepository.toGitPath(uptFile) _ <- writeXml( uptFile, activeTechniqueSerialisation.serialise(activeTechnique), @@ -775,11 +796,11 @@ class GitActiveTechniqueArchiverImpl( callbacks <- ZIO.foreach(uptModificationCallback.toList)(_.onArchive(activeTechnique, parents, gitCommit)) _ <- gitCommit match { case Some((modId, commiter, reason)) => - commitAddFileWithModId( - modId, - commiter, - gitPath, - s"Archive of technique library template for technique name '${activeTechnique.techniqueName.value}'${GET(reason)}" + gitConfigItemRepository.commitAddFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = s"Archive of technique library template for technique name '${activeTechnique.techniqueName.value}'${GET(reason)}" ) case None => ZIO.unit } @@ -801,15 +822,15 @@ class GitActiveTechniqueArchiverImpl( // don't forget to delete the category *directory* _ <- IOResult.attempt(FileUtils.forceDelete(atFile)) _ = logPure.debug(s"Deleted archived technique library template: ${atFile.getPath}") - gitPath = toGitPath(atFile) + gitPath = gitConfigItemRepository.toGitPath(atFile) _ <- ZIO.foreach(uptModificationCallback.toList)(_.onDelete(ptName, parents, None)) _ <- gitCommit match { case Some((modId, commiter, reason)) => - commitRmFileWithModId( - modId, - commiter, - gitPath, - s"Delete archive of technique library template for technique name '${ptName.value}'${GET(reason)}" + gitConfigItemRepository.commitRmFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = s"Delete archive of technique library template for technique name '${ptName.value}'${GET(reason)}" ) case None => ZIO.unit } @@ -817,7 +838,7 @@ class GitActiveTechniqueArchiverImpl( GitPath(gitPath) } } else { - GitPath(toGitPath(atFile)).succeed + GitPath(gitConfigItemRepository.toGitPath(atFile)).succeed } } yield { res @@ -854,11 +875,11 @@ class GitActiveTechniqueArchiverImpl( archived <- archiveActiveTechnique(activeTechnique, newParents, gitCommit) commited <- gitCommit match { case Some((modId, commiter, reason)) => - commitMvDirectoryWithModId( + gitConfigItemRepository.commitMvDirectoryWithModId( modId, commiter, - toGitPath(oldActiveTechniqueDirectory), - toGitPath(newActiveTechniqueDirectory), + gitConfigItemRepository.toGitPath(oldActiveTechniqueDirectory), + gitConfigItemRepository.toGitPath(newActiveTechniqueDirectory), "Move active technique for technique name '%s'%s".format( activeTechnique.techniqueName.value, GET(reason) @@ -877,19 +898,24 @@ class GitActiveTechniqueArchiverImpl( * A specific trait to create archive of an active technique. */ class GitDirectiveArchiverImpl( - override val gitRepo: GitRepositoryProvider, + gitRepo: GitRepositoryProvider, directiveSerialisation: DirectiveSerialisation, techniqueLibraryRootDir: String, // relative path ! - override val xmlPrettyPrinter: RudderPrettyPrinter, - override val gitModificationRepository: GitModificationRepository, - override val encoding: String, - override val groupOwner: String -) extends GitDirectiveArchiver with NamedZioLogger with GitConfigItemRepository with XmlArchiverUtils - with BuildCategoryPathName[ActiveTechniqueCategoryId] { + override val xmlPrettyPrinter: RudderPrettyPrinter, + gitModificationRepository: GitModificationRepository, + override val encoding: String, + override val groupOwner: String +) extends GitDirectiveArchiver with NamedZioLogger with XmlArchiverUtils with BuildCategoryPathName[ActiveTechniqueCategoryId] { + + private lazy val relativePath = techniqueLibraryRootDir + + private val gitConfigItemRepository: GitConfigItemRepository = + new GitConfigItemRepository(gitRepo, relativePath, gitModificationRepository) + + override def getItemDirectory: File = gitConfigItemRepository.getItemDirectory override def loggerName: String = this.getClass.getName - override lazy val relativePath = techniqueLibraryRootDir override def getCategoryName(categoryId: ActiveTechniqueCategoryId) = categoryId.value private def newPiFile( @@ -918,7 +944,7 @@ class GitDirectiveArchiverImpl( for { piFile <- newPiFile(directive.id.uid, ptName, catIds) - gitPath = toGitPath(piFile) + gitPath = gitConfigItemRepository.toGitPath(piFile) archive <- writeXml( piFile, directiveSerialisation.serialise(ptName, Some(variableRootSection), directive), @@ -926,11 +952,11 @@ class GitDirectiveArchiverImpl( ) commit <- gitCommit match { case Some((modId, commiter, reason)) => - commitAddFileWithModId( - modId, - commiter, - gitPath, - "Archive directive with ID '%s'%s".format(directive.id.uid.value, GET(reason)) + gitConfigItemRepository.commitAddFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = "Archive directive with ID '%s'%s".format(directive.id.uid.value, GET(reason)) ) case None => ZIO.unit } @@ -957,14 +983,14 @@ class GitDirectiveArchiverImpl( for { _ <- IOResult.attempt(FileUtils.forceDelete(piFile)) _ <- logPure.debug(s"Deleted archive of directive: '${piFile.getPath}'") - gitPath = toGitPath(piFile) + gitPath = gitConfigItemRepository.toGitPath(piFile) _ <- gitCommit match { case Some((modId, commiter, reason)) => - commitRmFileWithModId( - modId, - commiter, - gitPath, - s"Delete archive of directive with ID '${directiveId.value}'${GET(reason)}" + gitConfigItemRepository.commitRmFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = s"Delete archive of directive with ID '${directiveId.value}'${GET(reason)}" ) case None => ZIO.unit } @@ -972,7 +998,7 @@ class GitDirectiveArchiverImpl( GitPath(gitPath) } } else { - GitPath(toGitPath(piFile)).succeed + GitPath(gitConfigItemRepository.toGitPath(piFile)).succeed } } yield { res @@ -1001,11 +1027,17 @@ class GitNodeGroupArchiverImpl( override val encoding: String, serializedCategoryName: String, override val groupOwner: String -) extends GitNodeGroupArchiver with NamedZioLogger with GitConfigItemRepository with XmlArchiverUtils - with BuildCategoryPathName[NodeGroupCategoryId] with GitArchiverFullCommitUtils { +) extends GitNodeGroupArchiver with NamedZioLogger with XmlArchiverUtils with BuildCategoryPathName[NodeGroupCategoryId] + with GitArchiverFullCommitUtils { - override def loggerName: String = this.getClass.getName override lazy val relativePath = groupLibraryRootDir + + private val gitConfigItemRepository: GitConfigItemRepository = + new GitConfigItemRepository(gitRepo, relativePath, gitModificationRepository) + + override def getItemDirectory: File = gitConfigItemRepository.getItemDirectory + + override def loggerName: String = this.getClass.getName override def getCategoryName(categoryId: NodeGroupCategoryId) = categoryId.value override lazy val tagPrefix = "archives/groups/" @@ -1027,14 +1059,14 @@ class GitNodeGroupArchiverImpl( nodeGroupCategorySerialisation.serialise(ngc), "Archived node group category: " + ngcFile.getPath ) - gitPath = toGitPath(ngcFile) + gitPath = gitConfigItemRepository.toGitPath(ngcFile) commit <- gitCommit match { case Some((modId, commiter, reason)) => - commitAddFileWithModId( - modId, - commiter, - gitPath, - "Archive of node group category with ID '%s'%s".format(ngc.id.value, GET(reason)) + gitConfigItemRepository.commitAddFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = "Archive of node group category with ID '%s'%s".format(ngc.id.value, GET(reason)) ) case None => ZIO.unit } @@ -1050,7 +1082,7 @@ class GitNodeGroupArchiverImpl( ): IOResult[GitPath] = { for { ngcFile <- newNgFile(ngcId, getParents) - gitPath = toGitPath(ngcFile) + gitPath = gitConfigItemRepository.toGitPath(ngcFile) // don't forget to delete the category *directory* _ <- ZIO.whenZIO(IOResult.attempt(ngcFile.exists)) { for { @@ -1058,11 +1090,11 @@ class GitNodeGroupArchiverImpl( _ <- logPure.debug(s"Deleted archived node group category: ${ngcFile.getPath}") _ <- gitCommit match { case Some((modId, commiter, reason)) => - commitRmFileWithModId( - modId, - commiter, - gitPath, - s"Delete archive of node group category with ID '${ngcId.value}'${GET(reason)}" + gitConfigItemRepository.commitRmFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = s"Delete archive of node group category with ID '${ngcId.value}'${GET(reason)}" ) case None => ZIO.unit } @@ -1116,17 +1148,17 @@ class GitNodeGroupArchiverImpl( } commit <- gitCommit match { case Some((modId, commiter, reason)) => - commitMvDirectoryWithModId( - modId, - commiter, - toGitPath(oldNgcDir), - toGitPath(newNgcDir), - "Move archive of node group category with ID '%s'%s".format(ngc.id.value, GET(reason)) + gitConfigItemRepository.commitMvDirectoryWithModId( + modId = modId, + commiter = commiter, + oldGitPath = gitConfigItemRepository.toGitPath(oldNgcDir), + newGitPath = gitConfigItemRepository.toGitPath(newNgcDir), + commitMessage = "Move archive of node group category with ID '%s'%s".format(ngc.id.value, GET(reason)) ) case None => ZIO.unit } } yield { - GitPath(toGitPath(archive)) + GitPath(gitConfigItemRepository.toGitPath(archive)) } } } @@ -1174,16 +1206,16 @@ class GitNodeGroupArchiverImpl( ) commit <- gitCommit match { case Some((modId, commiter, reason)) => - commitAddFileWithModId( - modId, - commiter, - toGitPath(ngFile), - "Archive of node group with ID '%s'%s".format(ng.id.withDefaultRev.serialize, GET(reason)) + gitConfigItemRepository.commitAddFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitConfigItemRepository.toGitPath(ngFile), + commitMessage = "Archive of node group with ID '%s'%s".format(ng.id.withDefaultRev.serialize, GET(reason)) ) case None => ZIO.unit } } yield { - GitPath(toGitPath(archive)) + GitPath(gitConfigItemRepository.toGitPath(archive)) } } @@ -1194,7 +1226,7 @@ class GitNodeGroupArchiverImpl( ): IOResult[GitPath] = { for { ngFile <- newNgFile(ngId, getParents) - gitPath = toGitPath(ngFile) + gitPath = gitConfigItemRepository.toGitPath(ngFile) exists <- IOResult.attempt(ngFile.exists) res <- if (exists) { for { @@ -1203,11 +1235,11 @@ class GitNodeGroupArchiverImpl( _ <- logPure.debug(s"Deleted archived node group: ${ngFile.getPath}") _ <- gitCommit match { case Some((modId, commiter, reason)) => - commitRmFileWithModId( - modId, - commiter, - gitPath, - s"Delete archive of node group with ID '${ngId.withDefaultRev.serialize}'${GET(reason)}" + gitConfigItemRepository.commitRmFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = s"Delete archive of node group with ID '${ngId.withDefaultRev.serialize}'${GET(reason)}" ) case None => ZIO.unit } @@ -1244,17 +1276,17 @@ class GitNodeGroupArchiverImpl( } commit <- gitCommit match { case Some((modId, commiter, reason)) => - commitMvDirectoryWithModId( - modId, - commiter, - toGitPath(oldNgXmlFile), - toGitPath(newNgXmlFile), - "Move archive of node group with ID '%s'%s".format(ng.id.withDefaultRev.serialize, GET(reason)) + gitConfigItemRepository.commitMvDirectoryWithModId( + modId = modId, + commiter = commiter, + oldGitPath = gitConfigItemRepository.toGitPath(oldNgXmlFile), + newGitPath = gitConfigItemRepository.toGitPath(newNgXmlFile), + commitMessage = "Move archive of node group with ID '%s'%s".format(ng.id.withDefaultRev.serialize, GET(reason)) ) case None => ZIO.unit } } yield { - GitPath(toGitPath(archive)) + GitPath(gitConfigItemRepository.toGitPath(archive)) } } } @@ -1277,12 +1309,18 @@ class GitParameterArchiverImpl( override val gitModificationRepository: GitModificationRepository, override val encoding: String, override val groupOwner: String -) extends GitParameterArchiver with NamedZioLogger with GitConfigItemRepository with XmlArchiverUtils - with GitArchiverFullCommitUtils with BuildFilePaths[String] { +) extends GitParameterArchiver with NamedZioLogger with XmlArchiverUtils with GitArchiverFullCommitUtils + with BuildFilePaths[String] { - override def loggerName: String = this.getClass.getName override val relativePath = parameterRootDir - override val tagPrefix = "archives/parameters/" + + private val gitConfigItemRepository: GitConfigItemRepository = + new GitConfigItemRepository(gitRepo, relativePath, gitModificationRepository) + + override def getItemDirectory: File = gitConfigItemRepository.getItemDirectory + + override def loggerName: String = this.getClass.getName + override val tagPrefix = "archives/parameters/" override def getFileName(parameterName: String) = parameterName + ".xml" @@ -1292,7 +1330,7 @@ class GitParameterArchiverImpl( ): IOResult[GitPath] = { for { paramFile <- newFile(parameter.name).toIO - gitPath = toGitPath(paramFile) + gitPath = gitConfigItemRepository.toGitPath(paramFile) archive <- writeXml( paramFile, parameterSerialisation.serialise(parameter), @@ -1301,7 +1339,7 @@ class GitParameterArchiverImpl( commit <- doCommit match { case Some((modId, commiter, reason)) => val msg = "Archive parameter with name '%s'%s".format(parameter.name, GET(reason)) - commitAddFileWithModId(modId, commiter, gitPath, msg) + gitConfigItemRepository.commitAddFileWithModId(modId, commiter, gitPath, msg) case None => ZIO.unit } } yield { @@ -1325,18 +1363,18 @@ class GitParameterArchiverImpl( ): IOResult[GitPath] = { for { paramFile <- newFile(parameterName).toIO - gitPath = toGitPath(paramFile) + gitPath = gitConfigItemRepository.toGitPath(paramFile) _ <- ZIO.whenZIO(IOResult.attempt(paramFile.exists)) { for { deleted <- IOResult.attempt(FileUtils.forceDelete(paramFile)) _ <- logPure.debug(s"Deleted archive of parameter: ${paramFile.getPath}") _ <- doCommit match { case Some((modId, commiter, reason)) => - commitRmFileWithModId( - modId, - commiter, - gitPath, - s"Delete archive of parameter with name '${parameterName}'${GET(reason)}" + gitConfigItemRepository.commitRmFileWithModId( + modId = modId, + commiter = commiter, + gitPath = gitPath, + commitMessage = s"Delete archive of parameter with name '${parameterName}'${GET(reason)}" ) case None => ZIO.unit } diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/rule/category/GitRuleCategoryArchiver.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/rule/category/GitRuleCategoryArchiver.scala index fb02d36253..4a12b3b284 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/rule/category/GitRuleCategoryArchiver.scala +++ b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/rule/category/GitRuleCategoryArchiver.scala @@ -126,11 +126,17 @@ class GitRuleCategoryArchiverImpl( override val encoding: String, categoryFileName: String, override val groupOwner: String -) extends GitRuleCategoryArchiver with NamedZioLogger with GitConfigItemRepository with XmlArchiverUtils +) extends GitRuleCategoryArchiver with NamedZioLogger with XmlArchiverUtils with GitArchiverFullCommitUtils with BuildCategoryPathName[RuleCategoryId] { - override def loggerName: String = this.getClass.getName override val relativePath = ruleCategoryRootDir + + private val gitConfigItemRepository: GitConfigItemRepository = + new GitConfigItemRepository(gitRepo, relativePath, gitModificationRepository) + + override def getItemDirectory: File = gitConfigItemRepository.getItemDirectory + + override def loggerName: String = this.getClass.getName override val tagPrefix = "archives/configurations-rules/" def getCategoryName(categoryId: RuleCategoryId): String = categoryId.value @@ -146,7 +152,7 @@ class GitRuleCategoryArchiverImpl( // Build Rule category file, needs to reverse parents , start from end) for { ruleCategoryFile <- categoryFile(category.id, parents.reverse) - gitPath = toGitPath(ruleCategoryFile) + gitPath = gitConfigItemRepository.toGitPath(ruleCategoryFile) archive <- writeXml( ruleCategoryFile, ruleCategorySerialisation.serialise(category), @@ -155,7 +161,7 @@ class GitRuleCategoryArchiverImpl( commit <- gitCommit match { case Some((modId, commiter, reason)) => val commitMsg = s"Archive rule Category with ID '${category.id.value}' ${GET(reason)}" - commitAddFileWithModId(modId, commiter, gitPath, commitMsg) + gitConfigItemRepository.commitAddFileWithModId(modId, commiter, gitPath, commitMsg) case None => ZIO.unit } @@ -179,7 +185,7 @@ class GitRuleCategoryArchiverImpl( // Build Rule category file, needs to reverse parents , start from end) for { ruleCategoryFile <- categoryFile(categoryId, parents.reverse) - gitPath = toGitPath(ruleCategoryFile) + gitPath = gitConfigItemRepository.toGitPath(ruleCategoryFile) _ <- ZIO.whenZIO(IOResult.attempt(ruleCategoryFile.exists)) { for { _ <- IOResult.attempt(FileUtils.forceDelete(ruleCategoryFile)) @@ -187,7 +193,7 @@ class GitRuleCategoryArchiverImpl( _ <- doCommit match { case Some((modId, commiter, reason)) => val commitMsg = s"Delete archive of rule with ID '${categoryId.value} ${GET(reason)}" - commitRmFileWithModId(modId, commiter, gitPath, commitMsg) + gitConfigItemRepository.commitRmFileWithModId(modId, commiter, gitPath, commitMsg) case None => ZIO.unit } @@ -232,14 +238,14 @@ class GitRuleCategoryArchiverImpl( commit <- gitCommit match { case Some((modId, commiter, reason)) => val commitMsg = s"Move archive of rule category with ID '${category.id.value}'${GET(reason)}" - val oldPath = toGitPath(oldCategoryDir) - val newPath = toGitPath(newCategoryDir) - commitMvDirectoryWithModId(modId, commiter, oldPath, newPath, commitMsg) + val oldPath = gitConfigItemRepository.toGitPath(oldCategoryDir) + val newPath = gitConfigItemRepository.toGitPath(newCategoryDir) + gitConfigItemRepository.commitMvDirectoryWithModId(modId, commiter, oldPath, newPath, commitMsg) case None => ZIO.unit } } yield { - GitPath(toGitPath(archive)) + GitPath(gitConfigItemRepository.toGitPath(archive)) } } } diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala similarity index 92% rename from webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala rename to webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala index a4766e8cc0..e2f9501a87 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest2.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala @@ -51,7 +51,7 @@ import com.normation.eventlog.EventActor import com.normation.eventlog.ModificationId import com.normation.rudder.db.DB import com.normation.rudder.git.GitCommitId -import com.normation.rudder.git.GitConfigItemRepository +import com.normation.rudder.git.GitItemRepository import com.normation.rudder.git.GitRepositoryProvider import com.normation.rudder.git.GitRepositoryProviderImpl import com.normation.rudder.ncf.EditorTechnique @@ -100,7 +100,7 @@ object TempDir { } } -object JGitRepositoryTest2 extends ZIOSpecDefault { +object JGitRepositoryTest extends ZIOSpecDefault { def prettyPrinter: RudderPrettyPrinter = new RudderPrettyPrinter(Int.MaxValue, 2) @@ -187,7 +187,6 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { TechniqueArchiverImpl ] = ZLayer { for { - modRepo <- ZIO.service[GitModificationRepository] techniqueParse <- ZIO.service[TechniqueParser] techniqueCompiler <- ZIO.service[TechniqueCompiler] gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] @@ -196,7 +195,6 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { techniqueArchive = new TechniqueArchiverImpl( gitRepo = gitRepositoryProvider, xmlPrettyPrinter = prettyPrinter, - gitModificationRepository = modRepo, personIdentservice = personIdentservice, techniqueParser = techniqueParse, techniqueCompiler = techniqueCompiler, @@ -205,45 +203,39 @@ object JGitRepositoryTest2 extends ZIOSpecDefault { } yield techniqueArchive } - val gitConfigItemRepositoryLayer - : ZLayer[GitModificationRepository & GitRepositoryProvider, Nothing, GitConfigItemRepository] = { + val gitItemRepositoryLayer: ZLayer[GitModificationRepository & GitRepositoryProvider, Nothing, GitItemRepository] = { ZLayer { for { repository <- ZIO.service[GitRepositoryProvider] - modRepo <- ZIO.service[GitModificationRepository] - } yield new GitConfigItemRepository { - override val gitRepo: GitRepositoryProvider = repository - override def relativePath: String = "" - override def gitModificationRepository: GitModificationRepository = modRepo - } + } yield new GitItemRepository(gitRepo = repository, relativePath = "") } } val spec: Spec[Any, RudderError] = suite("The test lib")( - suite("GitConfigItemRepository")( + suite("GitItemRepository")( // to assess the usefulness of semaphore, you can remove `gitRepo.semaphore.withPermit` // in `commitAddFile` to check that you get the JGitInternalException. // More advanced tests may be needed to handle more complex cases of concurrent access, // see: https://issues.rudder.io/issues/19910 test("should not throw JGitInternalError on concurrent write") { - def addFileToRepositoryAndCommit(i: Int)(gitRoot: TempDir, archive: GitConfigItemRepository) = for { + def addFileToRepositoryAndCommit(i: Int)(gitRoot: TempDir, archive: GitItemRepository) = for { name <- zio.Random.nextString(8).map(s => i.toString + "_" + s) file = gitRoot.path / name _ <- IOResult.attempt(file.write("something in " + name)) actor = new PersonIdent("test", "test@test.com") - _ <- archive.commitAddFileWithModId(ModificationId(name), actor, name, "add " + name) + _ <- archive.commitAddFile(actor, name, "add " + name) } yield name for { gitRoot <- ZIO.service[TempDir] gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] - archive <- ZIO.service[GitConfigItemRepository] + archive <- ZIO.service[GitItemRepository] files <- ZIO.foreachPar(1 to 50)(i => addFileToRepositoryAndCommit(i)(gitRoot, archive)).withParallelism(16) created <- readElementsAt(gitRepositoryProvider.db, "refs/heads/master") } yield assert(created)(hasSameElements(files)) - }.provide(TempDir.layer, StubGitModificationRepository.layer, gitRepositoryProviderLayer, gitConfigItemRepositoryLayer) + }.provide(TempDir.layer, StubGitModificationRepository.layer, gitRepositoryProviderLayer, gitItemRepositoryLayer) ), suite("TechniqueArchiver.saveTechniqueCategory")( test("should create a new file and commit if the category does not exist") { diff --git a/webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/RudderConfig.scala b/webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/RudderConfig.scala index 69ffef5e1b..353f484ec1 100644 --- a/webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/RudderConfig.scala +++ b/webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/RudderConfig.scala @@ -1783,7 +1783,6 @@ object RudderConfigInit { lazy val techniqueArchiver = new TechniqueArchiverImpl( gitConfigRepo, prettyPrinter, - gitModificationRepository, personIdentService, techniqueParser, techniqueCompiler, diff --git a/webapp/sources/rudder/rudder-web/src/test/scala/bootstrap/liftweb/checks/migration/TestMigrateJsonTechniquesToYaml.scala b/webapp/sources/rudder/rudder-web/src/test/scala/bootstrap/liftweb/checks/migration/TestMigrateJsonTechniquesToYaml.scala index 835748db39..d3d69f47e9 100644 --- a/webapp/sources/rudder/rudder-web/src/test/scala/bootstrap/liftweb/checks/migration/TestMigrateJsonTechniquesToYaml.scala +++ b/webapp/sources/rudder/rudder-web/src/test/scala/bootstrap/liftweb/checks/migration/TestMigrateJsonTechniquesToYaml.scala @@ -43,30 +43,10 @@ import com.normation.eventlog.ModificationId import com.normation.inventory.domain.Version import com.normation.rudder.MockGitConfigRepo import com.normation.rudder.MockTechniques -import com.normation.rudder.db.DB import com.normation.rudder.facts.nodes.QueryContext -import com.normation.rudder.git.GitCommitId import com.normation.rudder.hooks.CmdResult -import com.normation.rudder.ncf.BundleName -import com.normation.rudder.ncf.CompilationResult -import com.normation.rudder.ncf.DeleteEditorTechnique -import com.normation.rudder.ncf.EditorTechniqueCompilationResult -import com.normation.rudder.ncf.EditorTechniqueReader -import com.normation.rudder.ncf.EditorTechniqueReaderImpl -import com.normation.rudder.ncf.GenericMethod -import com.normation.rudder.ncf.GitResourceFileService -import com.normation.rudder.ncf.ReadEditorTechniqueCompilationResult -import com.normation.rudder.ncf.ResourceFile -import com.normation.rudder.ncf.ResourceFileState -import com.normation.rudder.ncf.RuddercOptions -import com.normation.rudder.ncf.RuddercResult -import com.normation.rudder.ncf.RuddercService -import com.normation.rudder.ncf.RuddercTechniqueCompiler -import com.normation.rudder.ncf.TechniqueCompilationErrorsActorSync -import com.normation.rudder.ncf.TechniqueCompilationStatusService -import com.normation.rudder.ncf.TechniqueWriterImpl +import com.normation.rudder.ncf.* import com.normation.rudder.ncf.yaml.YamlTechniqueSerializer -import com.normation.rudder.repository.GitModificationRepository import com.normation.rudder.repository.xml.RudderPrettyPrinter import com.normation.rudder.repository.xml.TechniqueArchiverImpl import com.normation.rudder.repository.xml.TechniqueFiles @@ -151,11 +131,6 @@ class TestMigrateJsonTechniquesToYaml extends Specification with ContentMatchers val techniqueArchiver = new TechniqueArchiverImpl( gitMock.gitRepo, xmlPrettyPrinter, - new GitModificationRepository { - override def getCommits(modificationId: ModificationId): IOResult[Option[GitCommitId]] = None.succeed - override def addCommit(commit: GitCommitId, modId: ModificationId): IOResult[DB.GitCommitJoin] = - DB.GitCommitJoin(commit, modId).succeed - }, new TrivialPersonIdentService(), techMock.techniqueParser, techniqueCompiler, From 4a3ba7101370bfbd271c25723212c9f066869e61 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 14:24:51 +0100 Subject: [PATCH 12/15] step 12: GitConfigItemRepository should not extend GitItemRepository because you don't want a caller to add files without modId management --- .../rudder/git/GitItemRepository.scala | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala index 69dee0ffb3..9c95e19019 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala +++ b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala @@ -198,6 +198,10 @@ class GitItemRepository( } +object GitItemRepository { + +} + /* * An extension of simple GitItemRepositoty that in addition knows how to link commitId and modId together. * Used for all configuration objects, but not for facts. @@ -206,7 +210,12 @@ class GitConfigItemRepository( gitRepo: GitRepositoryProvider, relativePath: String, gitModificationRepository: GitModificationRepository -) extends GitItemRepository(gitRepo, relativePath) { +) { + + private val gitItemRepository: GitItemRepository = new GitItemRepository(gitRepo, relativePath) + + final def toGitPath(fsPath: File): String = gitItemRepository.toGitPath(fsPath) + final def getItemDirectory: File = gitItemRepository.getItemDirectory /** * Files in gitPath are added. @@ -219,7 +228,7 @@ class GitConfigItemRepository( commitMessage: String ): IOResult[GitCommitId] = { for { - commit <- commitAddFile(commiter, gitPath, commitMessage) + commit <- gitItemRepository.commitAddFile(commiter, gitPath, commitMessage) mod <- gitModificationRepository.addCommit(commit, modId) } yield { commit @@ -237,7 +246,7 @@ class GitConfigItemRepository( commitMessage: String ): IOResult[GitCommitId] = { for { - commit <- commitRmFile(commiter, gitPath, commitMessage) + commit <- gitItemRepository.commitRmFile(commiter, gitPath, commitMessage) mod <- gitModificationRepository.addCommit(commit, modId) } yield { commit @@ -259,7 +268,7 @@ class GitConfigItemRepository( commitMessage: String ): IOResult[GitCommitId] = { for { - commit <- commitMvDirectory(commiter, oldGitPath, newGitPath, commitMessage) + commit <- gitItemRepository.commitMvDirectory(commiter, oldGitPath, newGitPath, commitMessage) mod <- gitModificationRepository.addCommit(commit, modId) } yield { commit From 622704400e297aa752e6430587b99dd5c0c1ac1c Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 14:35:34 +0100 Subject: [PATCH 13/15] step 13: eliminate `StubGitModificationRepository` dependency --- .../cfclerk/services/JGitRepositoryTest.scala | 68 +++++++------------ 1 file changed, 23 insertions(+), 45 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala index e2f9501a87..b5853e1bd4 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala @@ -49,8 +49,6 @@ import com.normation.errors.RudderError import com.normation.errors.effectUioUnit import com.normation.eventlog.EventActor import com.normation.eventlog.ModificationId -import com.normation.rudder.db.DB -import com.normation.rudder.git.GitCommitId import com.normation.rudder.git.GitItemRepository import com.normation.rudder.git.GitRepositoryProvider import com.normation.rudder.git.GitRepositoryProviderImpl @@ -58,7 +56,6 @@ import com.normation.rudder.ncf.EditorTechnique import com.normation.rudder.ncf.TechniqueCompilationOutput import com.normation.rudder.ncf.TechniqueCompiler import com.normation.rudder.ncf.TechniqueCompilerApp -import com.normation.rudder.repository.GitModificationRepository import com.normation.rudder.repository.xml.RudderPrettyPrinter import com.normation.rudder.repository.xml.TechniqueArchiver import com.normation.rudder.repository.xml.TechniqueArchiverImpl @@ -69,7 +66,6 @@ import org.eclipse.jgit.revwalk.RevWalk import org.eclipse.jgit.treewalk.TreeWalk import zio.Chunk import zio.System -import zio.ULayer import zio.ZIO import zio.ZLayer import zio.syntax.ToZio @@ -104,37 +100,24 @@ object JGitRepositoryTest extends ZIOSpecDefault { def prettyPrinter: RudderPrettyPrinter = new RudderPrettyPrinter(Int.MaxValue, 2) - object StubGitModificationRepository { - val layer: ULayer[GitModificationRepository] = ZLayer.succeed { - new GitModificationRepository { - override def getCommits(modificationId: ModificationId): IOResult[Option[GitCommitId]] = None.succeed - - override def addCommit(commit: GitCommitId, modId: ModificationId): IOResult[DB.GitCommitJoin] = - DB.GitCommitJoin(commit, modId).succeed - } - } - } - - val techniqueParserLayer: ULayer[TechniqueParser] = ZLayer.succeed { + private val techniqueParserLayer = ZLayer.succeed { val varParser = new VariableSpecParser new TechniqueParser(varParser, new SectionSpecParser(varParser), new SystemVariableSpecServiceImpl()) } - object StubbedTechniqueCompiler { - val layer: ULayer[TechniqueCompiler] = ZLayer.succeed { - new TechniqueCompiler { - override def compileTechnique(technique: EditorTechnique): IOResult[TechniqueCompilationOutput] = { - TechniqueCompilationOutput(TechniqueCompilerApp.Rudderc, 0, Chunk.empty, "", "", "").succeed - } + private val stubbedTechniqueCompilerLayer = ZLayer.succeed { + new TechniqueCompiler { + override def compileTechnique(technique: EditorTechnique): IOResult[TechniqueCompilationOutput] = { + TechniqueCompilationOutput(TechniqueCompilerApp.Rudderc, 0, Chunk.empty, "", "", "").succeed + } - override def getCompilationOutputFile(technique: EditorTechnique): File = File("compilation-config.yml") + override def getCompilationOutputFile(technique: EditorTechnique): File = File("compilation-config.yml") - override def getCompilationConfigFile(technique: EditorTechnique): File = File("compilation-output.yml") - } + override def getCompilationConfigFile(technique: EditorTechnique): File = File("compilation-output.yml") } } - val gitRepositoryProviderLayer: ZLayer[TempDir, RudderError, GitRepositoryProviderImpl] = ZLayer { + private val gitRepositoryProviderLayer = ZLayer { for { gitRoot <- ZIO.service[TempDir] result <- GitRepositoryProviderImpl.make(gitRoot.path.pathAsString) @@ -175,35 +158,31 @@ object JGitRepositoryTest extends ZIOSpecDefault { // for test, we use as a group owner whatever git root directory has def currentUserName(repo: GitRepositoryProvider): GroupOwner = GroupOwner(repo.rootDirectory.groupName) - val currentUserNameLayer: ZLayer[GitRepositoryProvider, Nothing, GroupOwner] = ZLayer { + private val currentUserNameLayer = ZLayer { for { repo <- ZIO.service[GitRepositoryProvider] } yield currentUserName(repo) } - val techniqueArchiverLayer: ZLayer[ - GroupOwner & TrivialPersonIdentService & GitRepositoryProvider & TechniqueCompiler & TechniqueParser & GitModificationRepository, - Nothing, - TechniqueArchiverImpl - ] = ZLayer { + private val techniqueArchiverLayer = ZLayer { for { techniqueParse <- ZIO.service[TechniqueParser] techniqueCompiler <- ZIO.service[TechniqueCompiler] gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] personIdentservice <- ZIO.service[TrivialPersonIdentService] groupOwner <- ZIO.service[GroupOwner] - techniqueArchive = new TechniqueArchiverImpl( - gitRepo = gitRepositoryProvider, - xmlPrettyPrinter = prettyPrinter, - personIdentservice = personIdentservice, - techniqueParser = techniqueParse, - techniqueCompiler = techniqueCompiler, - groupOwner = groupOwner.value - ) - } yield techniqueArchive + } yield new TechniqueArchiverImpl( + gitRepo = gitRepositoryProvider, + xmlPrettyPrinter = prettyPrinter, + personIdentservice = personIdentservice, + techniqueParser = techniqueParse, + techniqueCompiler = techniqueCompiler, + groupOwner = groupOwner.value + ) + } - val gitItemRepositoryLayer: ZLayer[GitModificationRepository & GitRepositoryProvider, Nothing, GitItemRepository] = { + private val gitItemRepositoryLayer = { ZLayer { for { repository <- ZIO.service[GitRepositoryProvider] @@ -235,7 +214,7 @@ object JGitRepositoryTest extends ZIOSpecDefault { created <- readElementsAt(gitRepositoryProvider.db, "refs/heads/master") } yield assert(created)(hasSameElements(files)) - }.provide(TempDir.layer, StubGitModificationRepository.layer, gitRepositoryProviderLayer, gitItemRepositoryLayer) + }.provide(TempDir.layer, gitRepositoryProviderLayer, gitItemRepositoryLayer) ), suite("TechniqueArchiver.saveTechniqueCategory")( test("should create a new file and commit if the category does not exist") { @@ -284,9 +263,8 @@ object JGitRepositoryTest extends ZIOSpecDefault { } ).provide( TempDir.layer, - StubGitModificationRepository.layer, techniqueParserLayer, - StubbedTechniqueCompiler.layer, + stubbedTechniqueCompilerLayer, gitRepositoryProviderLayer, ZLayer.succeed(new TrivialPersonIdentService()), currentUserNameLayer, From e13185678d6149befae93f3a358f94da4fe1972f Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 14:45:15 +0100 Subject: [PATCH 14/15] step 14: split testsuite based on system-under-test --- .../cfclerk/services/JGitRepositoryTest.scala | 272 +++++++++--------- 1 file changed, 140 insertions(+), 132 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala index b5853e1bd4..490c29e0b8 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/cfclerk/services/JGitRepositoryTest.scala @@ -72,58 +72,45 @@ import zio.syntax.ToZio import zio.test.* import zio.test.Assertion.* -case class TempDir(path: File) - -object TempDir { - - /** - * Add a switch to be able to see tmp files (not clean temps) with - * -Dtests.clean.tmp=false - */ - val layer: ZLayer[Any, Nothing, TempDir] = ZLayer.scoped { - for { - keepFiles <- System.property("tests.clean.tmp").map(_.contains("false")).orDie - tempDir <- ZIO - .attemptBlockingIO(File.newTemporaryDirectory(prefix = "test-jgit-")) - .withFinalizer { dir => - (ZIO.logInfo(s"Deleting directory ${dir.path}") *> - ZIO.attemptBlocking(dir.delete(swallowIOExceptions = true))) - .unless(keepFiles) - .orDie - } - .orDie - } yield TempDir(tempDir) - } -} - object JGitRepositoryTest extends ZIOSpecDefault { + def spec: Spec[Any, RudderError] = suite("JGitRepositoryTest")(GitItemRepositoryTest.spec, TechniqueArchiverTest.spec) +} - def prettyPrinter: RudderPrettyPrinter = new RudderPrettyPrinter(Int.MaxValue, 2) +object GitItemRepositoryTest { + def spec: Spec[Any, RudderError] = suite("GitItemRepository")( + // to assess the usefulness of semaphore, you can remove `gitRepo.semaphore.withPermit` + // in `commitAddFile` to check that you get the JGitInternalException. + // More advanced tests may be needed to handle more complex cases of concurrent access, + // see: https://issues.rudder.io/issues/19910 + test("should not throw JGitInternalError on concurrent write") { - private val techniqueParserLayer = ZLayer.succeed { - val varParser = new VariableSpecParser - new TechniqueParser(varParser, new SectionSpecParser(varParser), new SystemVariableSpecServiceImpl()) - } + def addFileToRepositoryAndCommit(i: Int)(gitRoot: TempDir, archive: GitItemRepository) = for { + name <- zio.Random.nextString(8).map(s => i.toString + "_" + s) + file = gitRoot.path / name + _ <- IOResult.attempt(file.write("something in " + name)) + actor = new PersonIdent("test", "test@test.com") + _ <- archive.commitAddFile(actor, name, "add " + name) + } yield name - private val stubbedTechniqueCompilerLayer = ZLayer.succeed { - new TechniqueCompiler { - override def compileTechnique(technique: EditorTechnique): IOResult[TechniqueCompilationOutput] = { - TechniqueCompilationOutput(TechniqueCompilerApp.Rudderc, 0, Chunk.empty, "", "", "").succeed - } + for { + gitRoot <- ZIO.service[TempDir] + gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] + archive <- ZIO.service[GitItemRepository] + files <- ZIO.foreachPar((1 to 50))(i => addFileToRepositoryAndCommit(i)(gitRoot, archive)).withParallelism(16) + created <- readElementsAt(gitRepositoryProvider.db, "refs/heads/master") + } yield assert(created)(hasSameElements(files)) - override def getCompilationOutputFile(technique: EditorTechnique): File = File("compilation-config.yml") + }.provide(TempDir.layer, Layers.gitRepositoryProvider, gitItemRepositoryLayer) + ) - override def getCompilationConfigFile(technique: EditorTechnique): File = File("compilation-output.yml") + private val gitItemRepositoryLayer = { + ZLayer { + for { + repository <- ZIO.service[GitRepositoryProvider] + } yield new GitItemRepository(gitRepo = repository, relativePath = "") } } - private val gitRepositoryProviderLayer = ZLayer { - for { - gitRoot <- ZIO.service[TempDir] - result <- GitRepositoryProviderImpl.make(gitRoot.path.pathAsString) - } yield result - } - // listing files at a commit is complicated def readElementsAt(repository: Repository, commit: String): ZIO[Any, errors.SystemError, List[String]] = { val ref = repository.findRef(commit) @@ -148,16 +135,93 @@ object JGitRepositoryTest extends ZIOSpecDefault { ) } +} + +object TechniqueArchiverTest { + + def prettyPrinter: RudderPrettyPrinter = new RudderPrettyPrinter(Int.MaxValue, 2) + val category = TechniqueCategoryMetadata("My new category", "A new category", isSystem = false) val catPath = List("systemSettings", "myNewCategory") - - val modId = ModificationId("add-technique-cat") + val modId = ModificationId("add-technique-cat") case class GroupOwner(value: String) // for test, we use as a group owner whatever git root directory has def currentUserName(repo: GitRepositoryProvider): GroupOwner = GroupOwner(repo.rootDirectory.groupName) + def spec: Spec[Any, RudderError] = suite("TechniqueArchiver.saveTechniqueCategory")( + test("should create a new file and commit if the category does not exist") { + for { + gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] + techniqueArchive <- ZIO.service[TechniqueArchiver] + _ <- techniqueArchive + .saveTechniqueCategory( + catPath, + category, + modId, + EventActor("test"), + s"test: commit add category ${catPath.mkString("/")}" + ) + catFile = gitRepositoryProvider.rootDirectory / "techniques" / "systemSettings" / "myNewCategory" / "category.xml" + xml = catFile.contentAsString + lastCommitMsg = gitRepositoryProvider.git.log().setMaxCount(1).call().iterator().next().getFullMessage + } yield { + // note: no false ; it's only written when true + assert(xml)( + equalTo( + """ + | My new category + | A new category + |""".stripMargin + ) + ) && + assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) + } + }, + test("should do nothing when the category already exists") { + for { + gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] + techniqueArchive <- ZIO.service[TechniqueArchiver] + _ <- techniqueArchive + .saveTechniqueCategory( + catPath, + category, + modId, + EventActor("test"), + s"test: commit add category ${catPath.mkString("/")}" + ) + _ <- techniqueArchive.saveTechniqueCategory(catPath, category, modId, EventActor("test"), s"test: commit again") + lastCommitMsg = gitRepositoryProvider.git.log().setMaxCount(1).call().iterator().next().getFullMessage + } yield assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) + } + ).provide( + TempDir.layer, + techniqueParserLayer, + stubbedTechniqueCompilerLayer, + Layers.gitRepositoryProvider, + ZLayer.succeed(new TrivialPersonIdentService()), + currentUserNameLayer, + techniqueArchiverLayer + ) + + private val techniqueParserLayer = ZLayer.succeed { + val varParser = new VariableSpecParser + new TechniqueParser(varParser, new SectionSpecParser(varParser), new SystemVariableSpecServiceImpl()) + } + + private val stubbedTechniqueCompilerLayer = ZLayer.succeed { + new TechniqueCompiler { + override def compileTechnique(technique: EditorTechnique): IOResult[TechniqueCompilationOutput] = { + TechniqueCompilationOutput(TechniqueCompilerApp.Rudderc, 0, Chunk.empty, "", "", "").succeed + } + + override def getCompilationOutputFile(technique: EditorTechnique): File = File("compilation-config.yml") + + override def getCompilationConfigFile(technique: EditorTechnique): File = File("compilation-output.yml") + } + } + private val currentUserNameLayer = ZLayer { for { repo <- ZIO.service[GitRepositoryProvider] @@ -182,94 +246,38 @@ object JGitRepositoryTest extends ZIOSpecDefault { } - private val gitItemRepositoryLayer = { - ZLayer { - for { - repository <- ZIO.service[GitRepositoryProvider] - } yield new GitItemRepository(gitRepo = repository, relativePath = "") - } - } +} - val spec: Spec[Any, RudderError] = suite("The test lib")( - suite("GitItemRepository")( - // to assess the usefulness of semaphore, you can remove `gitRepo.semaphore.withPermit` - // in `commitAddFile` to check that you get the JGitInternalException. - // More advanced tests may be needed to handle more complex cases of concurrent access, - // see: https://issues.rudder.io/issues/19910 - test("should not throw JGitInternalError on concurrent write") { - - def addFileToRepositoryAndCommit(i: Int)(gitRoot: TempDir, archive: GitItemRepository) = for { - name <- zio.Random.nextString(8).map(s => i.toString + "_" + s) - file = gitRoot.path / name - _ <- IOResult.attempt(file.write("something in " + name)) - actor = new PersonIdent("test", "test@test.com") - _ <- archive.commitAddFile(actor, name, "add " + name) - } yield name +case class TempDir(path: File) - for { - gitRoot <- ZIO.service[TempDir] - gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] - archive <- ZIO.service[GitItemRepository] - files <- ZIO.foreachPar(1 to 50)(i => addFileToRepositoryAndCommit(i)(gitRoot, archive)).withParallelism(16) - created <- readElementsAt(gitRepositoryProvider.db, "refs/heads/master") - } yield assert(created)(hasSameElements(files)) - - }.provide(TempDir.layer, gitRepositoryProviderLayer, gitItemRepositoryLayer) - ), - suite("TechniqueArchiver.saveTechniqueCategory")( - test("should create a new file and commit if the category does not exist") { - for { - gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] - techniqueArchive <- ZIO.service[TechniqueArchiver] - _ <- techniqueArchive - .saveTechniqueCategory( - catPath, - category, - modId, - EventActor("test"), - s"test: commit add category ${catPath.mkString("/")}" - ) - catFile = gitRepositoryProvider.rootDirectory / "techniques" / "systemSettings" / "myNewCategory" / "category.xml" - xml = catFile.contentAsString - lastCommitMsg = gitRepositoryProvider.git.log().setMaxCount(1).call().iterator().next().getFullMessage - } yield { - // note: no false ; it's only written when true - assert(xml)( - equalTo( - """ - | My new category - | A new category - |""".stripMargin - ) - ) && - assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) - } - }, - test("should do nothing when the category already exists") { - for { - gitRepositoryProvider <- ZIO.service[GitRepositoryProvider] - techniqueArchive <- ZIO.service[TechniqueArchiver] - _ <- techniqueArchive - .saveTechniqueCategory( - catPath, - category, - modId, - EventActor("test"), - s"test: commit add category ${catPath.mkString("/")}" - ) - _ <- techniqueArchive.saveTechniqueCategory(catPath, category, modId, EventActor("test"), s"test: commit again") - lastCommitMsg = gitRepositoryProvider.git.log().setMaxCount(1).call().iterator().next().getFullMessage - } yield assert(lastCommitMsg)(equalTo("test: commit add category systemSettings/myNewCategory")) - } - ).provide( - TempDir.layer, - techniqueParserLayer, - stubbedTechniqueCompilerLayer, - gitRepositoryProviderLayer, - ZLayer.succeed(new TrivialPersonIdentService()), - currentUserNameLayer, - techniqueArchiverLayer - ) - ) +object TempDir { + + /** + * Add a switch to be able to see tmp files (not clean temps) with + * -Dtests.clean.tmp=false + */ + val layer: ZLayer[Any, Nothing, TempDir] = ZLayer.scoped { + for { + keepFiles <- System.property("tests.clean.tmp").map(_.contains("false")).orDie + tempDir <- ZIO + .attemptBlockingIO(File.newTemporaryDirectory(prefix = "test-jgit-")) + .withFinalizer { dir => + (ZIO.logInfo(s"Deleting directory ${dir.path}") *> + ZIO.attemptBlocking(dir.delete(swallowIOExceptions = true))) + .unless(keepFiles) + .orDie + } + .orDie + } yield TempDir(tempDir) + } +} + +object Layers { + private[services] val gitRepositoryProvider = ZLayer { + for { + gitRoot <- ZIO.service[TempDir] + result <- GitRepositoryProviderImpl.make(gitRoot.path.pathAsString) + } yield result + } } From 1ca62e9cf94ee20d53e3a1211c637273eb0e2254 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Thu, 14 Nov 2024 15:06:13 +0100 Subject: [PATCH 15/15] apply spotless --- .../com/normation/rudder/git/GitItemRepository.scala | 4 +--- .../rudder/repository/xml/GitArchivers.scala | 12 ++++++++---- .../rule/category/GitRuleCategoryArchiver.scala | 6 +++--- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala index 9c95e19019..b4bdd21335 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala +++ b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/git/GitItemRepository.scala @@ -198,9 +198,7 @@ class GitItemRepository( } -object GitItemRepository { - -} +object GitItemRepository {} /* * An extension of simple GitItemRepositoty that in addition knows how to link commitId and modId together. diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/xml/GitArchivers.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/xml/GitArchivers.scala index 2f19446763..eeabedaf9d 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/xml/GitArchivers.scala +++ b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/xml/GitArchivers.scala @@ -800,7 +800,8 @@ class GitActiveTechniqueArchiverImpl( modId = modId, commiter = commiter, gitPath = gitPath, - commitMessage = s"Archive of technique library template for technique name '${activeTechnique.techniqueName.value}'${GET(reason)}" + commitMessage = + s"Archive of technique library template for technique name '${activeTechnique.techniqueName.value}'${GET(reason)}" ) case None => ZIO.unit } @@ -830,7 +831,8 @@ class GitActiveTechniqueArchiverImpl( modId = modId, commiter = commiter, gitPath = gitPath, - commitMessage = s"Delete archive of technique library template for technique name '${ptName.value}'${GET(reason)}" + commitMessage = + s"Delete archive of technique library template for technique name '${ptName.value}'${GET(reason)}" ) case None => ZIO.unit } @@ -1239,7 +1241,8 @@ class GitNodeGroupArchiverImpl( modId = modId, commiter = commiter, gitPath = gitPath, - commitMessage = s"Delete archive of node group with ID '${ngId.withDefaultRev.serialize}'${GET(reason)}" + commitMessage = + s"Delete archive of node group with ID '${ngId.withDefaultRev.serialize}'${GET(reason)}" ) case None => ZIO.unit } @@ -1281,7 +1284,8 @@ class GitNodeGroupArchiverImpl( commiter = commiter, oldGitPath = gitConfigItemRepository.toGitPath(oldNgXmlFile), newGitPath = gitConfigItemRepository.toGitPath(newNgXmlFile), - commitMessage = "Move archive of node group with ID '%s'%s".format(ng.id.withDefaultRev.serialize, GET(reason)) + commitMessage = + "Move archive of node group with ID '%s'%s".format(ng.id.withDefaultRev.serialize, GET(reason)) ) case None => ZIO.unit } diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/rule/category/GitRuleCategoryArchiver.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/rule/category/GitRuleCategoryArchiver.scala index 4a12b3b284..4095034b5c 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/rule/category/GitRuleCategoryArchiver.scala +++ b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/rule/category/GitRuleCategoryArchiver.scala @@ -126,8 +126,8 @@ class GitRuleCategoryArchiverImpl( override val encoding: String, categoryFileName: String, override val groupOwner: String -) extends GitRuleCategoryArchiver with NamedZioLogger with XmlArchiverUtils - with GitArchiverFullCommitUtils with BuildCategoryPathName[RuleCategoryId] { +) extends GitRuleCategoryArchiver with NamedZioLogger with XmlArchiverUtils with GitArchiverFullCommitUtils + with BuildCategoryPathName[RuleCategoryId] { override val relativePath = ruleCategoryRootDir @@ -137,7 +137,7 @@ class GitRuleCategoryArchiverImpl( override def getItemDirectory: File = gitConfigItemRepository.getItemDirectory override def loggerName: String = this.getClass.getName - override val tagPrefix = "archives/configurations-rules/" + override val tagPrefix = "archives/configurations-rules/" def getCategoryName(categoryId: RuleCategoryId): String = categoryId.value