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 #19061: Use rudderc to compile technique from the editor instead of rudder logic #3553

Conversation

VinceMacBuche
Copy link
Member

@VinceMacBuche VinceMacBuche requested a review from fanf March 23, 2021 18:27
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.

Some change should be done to allow testing and better debugging. Perhaps they can be done in a second PR, but I fear we won't do them at all in that case. I let you decide what you think is the best (you can selfmerge in the corresponding case).

@@ -330,9 +350,12 @@ class TechniqueWriter (
}
}

def techniquePath(technique : Technique) = s"techniques/${technique.category}/${technique.bundleName.value}/${technique.version.value}"
Copy link
Member

Choose a reason for hiding this comment

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

What happens with category if it's a sub category? Does is contains the full relative path?
Also, I think we are going to have problem by calling Technique both com.normation.rudder.ncf.Technique and com.normation.cfclerk.domain.Technique. Can we rename the first one EditorTechnique ? (or something letting us know that it's not the same).

for {
_ <- ApplicationLoggerPure.info(s"An error occurred when compiling technique '${technique.name}' (id : '${technique.bundleName}') with rudderc, error details will be displayed below, falling back to old saving process")
_ <- ApplicationLoggerPure.error(s"Rudderc error when compiling technique '${technique.name}' (id : '${technique.bundleName}'): ${e.fullMsg}")
- <- writeAgentFiles (technique, methods, modId, committer)
Copy link
Member

Choose a reason for hiding this comment

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

Nice :)
Perhaps we should store these rudderc exception in dedicated, independendant log files, for ex in /var/log/rudder/rudderc/failures/YYYY-MM-dd-hhmmss-techniqueName.log with the context that lead to the error:

  • json file
  • if it exists, cf dsc file
  • the error log

That would allow users to send us these reports (or even to get them automatically with the support script)

case Left(error: Error) => ApplicationLogger.error(error.getMessage)
case Left(error: RudderError) => ApplicationLogger.error(error.msg)
case Right(_) => ApplicationLogger.info(s"rudder-lang tester successfully looped for technique ${technique.bundleName.value}")
def callRudderc(technique : Technique) = {
Copy link
Member

Choose a reason for hiding this comment

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

I believe that needs to be an independant service, with constructor argument:

  • config path file
  • rudderc binary path
  • target base path

Else we won't be able to test properties on the outputs at all.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding testing, we will need to embed a rudderc at some point - but I wonder where is the limit between testing rudderc and testing integration of rudderc into rudder with the specific things for it.

Copy link
Member

Choose a reason for hiding this comment

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

(we need at least to test the general output layout, path, etc)

@VinceMacBuche
Copy link
Member Author

PR rebased

@VinceMacBuche VinceMacBuche force-pushed the ust_19061/use_rudderc_to_compile_technique_from_the_editor_instead_of_rudder_logic branch from 0ae62fb to 266b849 Compare June 3, 2021 08:30
… instead of rudder logic

Fixes #19061: Use rudderc to compile technique from the editor instead of rudder logic
@VinceMacBuche
Copy link
Member Author

PR updated with a new commit

@@ -74,7 +74,7 @@ import com.normation.rudder.ncf.MethodCall
import com.normation.rudder.ncf.ParameterId
import com.normation.rudder.ncf.MethodParameter
import com.normation.rudder.ncf.TechniqueParameter
import com.normation.rudder.ncf.{Technique => NcfTechnique}
import com.normation.rudder.ncf.{EditorTechnique => NcfTechnique}
Copy link
Member

Choose a reason for hiding this comment

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

you should use EditorTechnique everywhere here, not NcfTechnique

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.

Just a s/NcfTechnique/EditorTechnique and you can merge it.

… editor instead of rudder logic

Fixes #19061: Use rudderc to compile technique from the editor instead of rudder logic
@VinceMacBuche
Copy link
Member Author

PR updated with a new commit

@VinceMacBuche VinceMacBuche merged commit 6265ecb into Normation:master Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants