[WFCORE-6349] Log yaml files used for configuration extension #5582
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello, The-Huginn. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment. |
|
/ok-to-test |
yersan
reviewed
Jul 11, 2023
| this.configs.add(config); | ||
| } | ||
| } else { | ||
| throw MGMT_OP_LOGGER.missingYamlFile(file != null ? file.toAbsolutePath().toString() : ""); | ||
| } | ||
| } | ||
| MGMT_OP_LOGGER.loadingYamlFiles(System.currentTimeMillis() - start); | ||
| MGMT_OP_LOGGER.loadingYamlFiles(System.currentTimeMillis() - start, String.join(",", parsedFiles)); |
There was a problem hiding this comment.
I'm not very familiar with yaml configuration, but since now the log level is INFO, would this load method be called always even if there are no files to process? I'm just wondering if we are going to have always a log trace independently if there are files parsed.
There was a problem hiding this comment.
Why use a list for this since we know the number of files
There was a problem hiding this comment.
I used list as we know only maximum number of files. They still need to contain the valid root node, which is being checked. I can redo it into array however an additional variable for iterating the array will be needed.
There was a problem hiding this comment.
Ok, Let's keep the list
@yersan the extension is loaded only if there are configuration files
github-actions
bot
added
the
deps-ok
ehsavoie
suggested changes
Jul 11, 2023
| @@ -115,13 +116,14 @@ private void load() { | |||
| for (String excluded : EXCLUDED_ELEMENTS) { | |||
| config.remove(excluded); | |||
| } | |||
| parsedFiles.add(file.toFile().getName()); | |||
There was a problem hiding this comment.
It should put the absolute path if the gaol is to know which files were used
There was a problem hiding this comment.
I will update it
| this.configs.add(config); | ||
| } | ||
| } else { | ||
| throw MGMT_OP_LOGGER.missingYamlFile(file != null ? file.toAbsolutePath().toString() : ""); | ||
| } | ||
| } | ||
| MGMT_OP_LOGGER.loadingYamlFiles(System.currentTimeMillis() - start); | ||
| MGMT_OP_LOGGER.loadingYamlFiles(System.currentTimeMillis() - start, String.join(",", parsedFiles)); |
There was a problem hiding this comment.
Why use a list for this since we know the number of files
ehsavoie
reviewed
Jul 11, 2023
| @LogMessage(level = DEBUG) | ||
| @Message(id = 487, value = "It took %s ms to load and parse the yaml files") | ||
| void loadingYamlFiles(long duration); | ||
| @LogMessage(level = INFO) |
There was a problem hiding this comment.
This should be left in DEBUG
|
Core -> Full Integration Build 12763 outcome was FAILURE using a merge of f8f20b1 Failed tests |
|
Core -> Full Integration Build 12565 outcome was FAILURE using a merge of 100ebc9 Failed tests |
|
@ehsavoie Do you agree with the current status? |
ehsavoie
approved these changes
Aug 11, 2023
yersan
added
the
ready-for-merge
|
Thanks @The-Huginn and @ehsavoie |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.redhat.com/browse/WFCORE-6349