Skip to content

Fix config for Trace level logging within the ConfigBuilder class #6040

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix config for Trace level logging within the ConfigBuilder class #6040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
38be4e7
Fix config for Trace level logging within the ConfigBuilder class (Is…
kdesnos May 6, 2025
367da77
Merge branch 'master' into fixConfigBuilderTrace
kdesnos May 6, 2025
8e37099
Merge branch 'master' into fixConfigBuilderTrace
kdesnos May 6, 2025
e198ef2
Ignore a test within BatchLoggingTest when GOOGLE_APPLICATION_CREDENT…
kdesnos May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion modules/nextflow/src/main/groovy/nextflow/cli/CmdRun.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import groovy.json.JsonSlurper
import groovy.transform.CompileStatic
import groovy.transform.Memoized
import groovy.util.logging.Slf4j
import org.slf4j.LoggerFactory
import groovyx.gpars.GParsConfig
import nextflow.BuildInfo
import nextflow.NF
Expand Down Expand Up @@ -355,7 +356,8 @@ class CmdRun extends CmdBase implements HubOptions {

final isTowerEnabled = config.navigate('tower.enabled') as Boolean
final isDataEnabled = config.navigate("lineage.enabled") as Boolean
if( isTowerEnabled || isDataEnabled || log.isTraceEnabled() )

if( isTowerEnabled || isDataEnabled || LoggerFactory.getLogger(nextflow.config.ConfigBuilder).isTraceEnabled() )
Comment on lines +359 to +360
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 we can just remove these conditions and resolve the config always

Copy link
Member

Choose a reason for hiding this comment

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

I never understood why we resolve it here anyway, when it is already resolved here

nextflow/modules/nextflow/src/main/groovy/nextflow/cli/CmdRun.groovy

Lines 329 to 333 in 2c9dd9b

final builder = new ConfigBuilder()
.setOptions(launcher.options)
.setCmdRun(this)
.setBaseDir(scriptFile.parent)
final config = builder .build()

Copy link
Member

Choose a reason for hiding this comment

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

Because this it's done to render it as a string using

nextflow/modules/nextflow/src/main/groovy/nextflow/config/ConfigBuilder.groovy

Lines 908 to 909 in e4af230

.setShowClosures(true)
.setStripSecrets(true)

maybe it can be done reusing the config object already build, but never looked into that

runner.session.resolvedConfig = ConfigBuilder.resolveConfig(scriptFile.parent, this)
// note config files are collected during the build process
// this line should be after `ConfigBuilder#build`
Expand Down
1 change: 1 addition & 0 deletions plugins/nf-google/src/test/nextflow/cloud/google/batch/logging/BatchLoggingTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import spock.lang.Timeout
@Slf4j
class BatchLoggingTest extends Specification {

@Requires({System.getenv('GOOGLE_APPLICATION_CREDENTIALS')})
def 'should parse stdout and stderr' () {
given:
def OUT_ENTRY1 = LogEntry.newBuilder(StringPayload.of('No user sessions are running outdated binaries.\n')).setSeverity(Severity.INFO).build()
Expand Down
Loading