Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #17568: Allow to regenerate generic methods when there is a new one #3024

Conversation

VinceMacBuche
Copy link
Member

@VinceMacBuche VinceMacBuche requested a review from fanf May 28, 2020 22:36
@@ -53,11 +53,27 @@ class TechniqueReader(
}
}

def doesMethodsMetadataFileNeedsUpdate ={
def getCfFilesInDir(file : File): Iterator[File] = {
if (file.isDirectory) {
Copy link
Member

Choose a reason for hiding this comment

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

why not file.collectChildren(your extension filter) ?


val methodsFileModifiedTime = methodsFile.lastModifiedTime()

(getCfFilesInDir(baseDir) ++ getCfFilesInDir(userDir)).map{_.lastModifiedTime()}.exists(_.isAfter(methodsFileModifiedTime))
Copy link
Member

Choose a reason for hiding this comment

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

That seems inefficient. You could either collect directly max time when you iterate on childrens (map is not lazy, so you will go through all children for it), or even pass the comparison date in your collect's filter so that you just have to check for iterator.nonEmpty (which should stop on first match)

@VinceMacBuche
Copy link
Member Author

PR updated with a new commit

Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

I think the logic is OK and you can self merge. I added a comment to help you see how you could make the code easier to test, but it's not a required change, I let you decide.

val baseDir = root / "usr" / "share" / "ncf" / "tree" / "30_generic_methods"
val userDir = configuration_repository / "ncf" / "30_generic_methods"
baseDir.collectChildren(isAMethodNewerThanCache).isEmpty || userDir.collectChildren(isAMethodNewerThanCache).isEmpty
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok but it's hard to test as it is. What would make it easier to test is to have it split in two:

  • one part with the logic and parameters for all states,
  • one methods that set state for user convenience, ie:
 // this one contains logic but stateless and so easy to test - actually, we could make it pure
 // with an `IORestul.effect` around last statement
  private[this] def checkNeedsUpdate(lastTime: TimeType, dirs: List[File]) : Boolean = {
    def isAMethodNewerThanCache(file : File) : Boolean = {
      file.isRegularFile && file.extension(false).map(_ == "cf").getOrElse(false)  && lastTime.isAfter(methodsFileModifiedTime)
    }
    dirs.exist(dir => dir.collectChildren(isAMethodNewerThanCache).isEmpty)
  }
  // this one is convenient and meaningful to use in the code (but untestable)
  private[this] def doesMethodsMetadataFileNeedsUpdate : Boolean = {
    val methodsFileModifiedTime = methodsFile.lastModifiedTime()
    val baseDir = root / "usr" / "share" / "ncf" / "tree" / "30_generic_methods"
    val userDir = configuration_repository / "ncf" / "30_generic_methods"
   checkNeedsUpdate(methodsFileModifiedTime, baseDir :: userDir :: Nil)
  }

@VinceMacBuche
Copy link
Member Author

PR updated with a new commit

for {
metadataFileExists <- IOResult.effect(methodsFile.exists)
needsUpdate <- if (metadataFileExists) {
val methodsFileModifiedTime = methodsFile.lastModifiedTime()
Copy link
Member

Choose a reason for hiding this comment

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

this is an IO too :)

@VinceMacBuche
Copy link
Member Author

PR updated with a new commit

@Normation-Quality-Assistant
Copy link
Contributor

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

@fanf
Copy link
Member

fanf commented Jun 1, 2020

OK, squash merging this PR

@fanf fanf force-pushed the bug_17568/allow_to_regenerate_generic_methods_when_there_is_a_new_one branch from 70d863c to 188cff0 Compare June 1, 2020 18:50
@fanf fanf merged commit 188cff0 into Normation:branches/rudder/6.1 Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants