ts-41920 move config id validation to ignore order of options

CHANGELOG.md

@@ -6,6 +6,8 @@ We use [semantic versioning](http://semver.org/):
 
 # Next version
 - [fix] _agent_: Fix wrong class description of GitMultiProjectPropertiesLocator in logs.
+- [fix] _agent_: Specifying `config-id` before Teamscale credentials led to option parsing errors. 
+
 # 34.2.2
 - [fix] _teamscale-gradle-plugin_: 401 Unauthorized error when trying to upload reports to Teamscale 
 

agent/src/main/java/com/teamscale/jacoco/agent/PreMain.java

@@ -2,7 +2,10 @@
 
 import com.teamscale.client.HttpUtils;
 import com.teamscale.jacoco.agent.configuration.AgentOptionReceiveException;
+import com.teamscale.jacoco.agent.logging.DebugLogDirectoryPropertyDefiner;
+import com.teamscale.jacoco.agent.logging.LogDirectoryPropertyDefiner;
 import com.teamscale.jacoco.agent.logging.LogToTeamscaleAppender;
+import com.teamscale.jacoco.agent.logging.LoggingUtils;
 import com.teamscale.jacoco.agent.options.AgentOptionParseException;
 import com.teamscale.jacoco.agent.options.AgentOptions;
 import com.teamscale.jacoco.agent.options.AgentOptionsParser;
@@ -13,9 +16,6 @@
 import com.teamscale.jacoco.agent.testimpact.TestwiseCoverageAgent;
 import com.teamscale.jacoco.agent.upload.UploaderException;
 import com.teamscale.jacoco.agent.util.AgentUtils;
-import com.teamscale.jacoco.agent.logging.DebugLogDirectoryPropertyDefiner;
-import com.teamscale.jacoco.agent.logging.LogDirectoryPropertyDefiner;
-import com.teamscale.jacoco.agent.logging.LoggingUtils;
 import com.teamscale.report.util.ILogger;
 import org.conqat.lib.commons.collections.CollectionUtils;
 import org.conqat.lib.commons.collections.Pair;
@@ -115,55 +115,58 @@
 		agent.registerShutdownHook();
 	}
 
 	private static Pair<AgentOptions, List<Exception>> getAndApplyAgentOptions(String options, String environmentConfigId,
 			String environmentConfigFile) throws AgentOptionParseException, IOException, AgentOptionReceiveException {
 
 		DelayedLogger delayedLogger = new DelayedLogger();
 		List<String> javaAgents = CollectionUtils.filter(ManagementFactory.getRuntimeMXBean().getInputArguments(),
 				s -> s.contains("-javaagent"));
 		// We allow multiple instances of the teamscale-jacoco-agent as we ensure with the #LOCKING_SYSTEM_PROPERTY to only use it once
 		if (!javaAgents.stream().allMatch(javaAgent -> javaAgent.contains("teamscale-jacoco-agent.jar"))) {
 			delayedLogger.warn("Using multiple java agents could interfere with coverage recording.");
 		}
 		if (!javaAgents.get(0).contains("teamscale-jacoco-agent.jar")) {
 			delayedLogger.warn("For best results consider registering the Teamscale JaCoCo Agent first.");
 		}
 
 		TeamscaleCredentials credentials = TeamscalePropertiesUtils.parseCredentials();
 		if (credentials == null) {
-			delayedLogger.warn("Did not find a teamscale.properties file!");
+			// As many users still don't use the installer based setup, this log message will be shown in almost every log.
+			// We use a debug log, as this message can be confusing for customers that think a teamscale.properties file is synonymous with a config file.
+			delayedLogger.debug(
+					"No explicit teamscale.properties file given. Looking for Teamscale credentials in a config file or via a command line argument. This is expected unless the installer based setup was used.");
 		}
 
 		Pair<AgentOptions, List<Exception>> parseResult;
 		AgentOptions agentOptions;
 		try {
 			parseResult = AgentOptionsParser.parse(
 					options, environmentConfigId, environmentConfigFile, credentials,
 					delayedLogger);
 			agentOptions = parseResult.getFirst();
 		} catch (AgentOptionParseException e) {
 			try (LoggingUtils.LoggingResources ignored = initializeFallbackLogging(options, delayedLogger)) {
 				delayedLogger.errorAndStdErr("Failed to parse agent options: " + e.getMessage(), e);
 				attemptLogAndThrow(delayedLogger);
 				throw e;
 			}
 		} catch (AgentOptionReceiveException e) {
 			try (LoggingUtils.LoggingResources ignored = initializeFallbackLogging(options, delayedLogger)) {
 				delayedLogger.errorAndStdErr(
 						e.getMessage() + " The application should start up normally, but NO coverage will be collected!",
 						e);
 				attemptLogAndThrow(delayedLogger);
 				throw e;
 			}
 		}
 
 		initializeLogging(agentOptions, delayedLogger);
 		Logger logger = LoggingUtils.getLogger(Agent.class);
 		delayedLogger.logTo(logger);
 		HttpUtils.setShouldValidateSsl(agentOptions.shouldValidateSsl());
 
 		return parseResult;
 	}
 
 	private static void attemptLogAndThrow(DelayedLogger delayedLogger) {
 		// We perform actual logging output after writing to console to

agent/src/main/java/com/teamscale/jacoco/agent/options/AgentOptionsParser.java

@@ -6,6 +6,7 @@
 package com.teamscale.jacoco.agent.options;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.teamscale.client.HttpUtils;
 import com.teamscale.client.ProxySystemProperties;
 import com.teamscale.client.StringUtils;
 import com.teamscale.client.TeamscaleProxySystemProperties;
@@ -24,6 +25,7 @@
 import org.conqat.lib.commons.collections.Pair;
 import org.conqat.lib.commons.filesystem.FileSystemUtils;
 
+import javax.annotation.Nullable;
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
@@ -68,7 +70,7 @@
 	 */
 	public static Pair<AgentOptions, List<Exception>> parse(String optionsString, String environmentConfigId,
 			String environmentConfigFile,
-			TeamscaleCredentials credentials,
+			@Nullable TeamscaleCredentials credentials,
 			ILogger logger) throws AgentOptionParseException, AgentOptionReceiveException {
 		AgentOptionsParser parser = new AgentOptionsParser(logger, environmentConfigId, environmentConfigFile,
 				credentials);
@@ -105,48 +107,48 @@
 	/**
 	 * Parses the given command-line options.
 	 */
 	/* package */ AgentOptions parse(
 			String optionsString) throws AgentOptionParseException, AgentOptionReceiveException {
 
 		if (optionsString == null) {
 			optionsString = "";
 		}
 		logger.debug("Parsing options: " + optionsString);
 
 		AgentOptions options = new AgentOptions(logger);
 		options.originalOptionsString = optionsString;
 
 		if (credentials != null) {
 			options.teamscaleServer.url = credentials.url;
 			options.teamscaleServer.userName = credentials.userName;
 			options.teamscaleServer.userAccessToken = credentials.accessKey;
 		}
 
 		if (!StringUtils.isEmpty(optionsString)) {
 			String[] optionParts = optionsString.split(",");
 			for (String optionPart : optionParts) {
 				try {
 					handleOptionPart(options, optionPart);
 				} catch (Exception e) {
 					collectedErrors.add(e);
 				}
 			}
 		}
 
 		// we have to put the proxy options into system properties before reading the configuration from Teamscale as we
 		// might need them to connect to Teamscale
 		putTeamscaleProxyOptionsIntoSystemProperties(options);
 
 		handleConfigId(options);
 		handleConfigFile(options);
 
 		Validator validator = options.getValidator();
 		if (!validator.isValid()) {
 			collectedErrors.add(new AgentOptionParseException("Invalid options given: " + validator.getErrorMessage()));
 		}
 
 		return options;
 	}
 
 	/**
 	 * Stores the agent options for proxies in the {@link TeamscaleProxySystemProperties} and overwrites the password
@@ -292,122 +294,119 @@
 	 *
 	 * @return true if it has successfully processed the given option.
 	 */
 	private boolean handleAgentOptions(AgentOptions options, String key, String value)
 			throws AgentOptionParseException, AgentOptionReceiveException {
 		switch (key) {
 			case "config-id":
-				storeConfigId(options, value);
+				options.teamscaleServer.configId = value;
 				return true;
 			case CONFIG_FILE_OPTION:
 				readConfigFromFile(options, parsePath(filePatternResolver, key, value).toFile());
 				return true;
 			case LOGGING_CONFIG_OPTION:
 				options.loggingConfig = parsePath(filePatternResolver, key, value);
 				return true;
 			case "interval":
 				options.dumpIntervalInMinutes = parseInt(key, value);
 				return true;
 			case "validate-ssl":
 				options.validateSsl = Boolean.parseBoolean(value);
 				return true;
 			case "out":
 				options.setParentOutputDirectory(parsePath(filePatternResolver, key, value));
 				return true;
 			case "upload-metadata":
 				try {
 					options.additionalMetaDataFiles = CollectionUtils.map(splitMultiOptionValue(value), Paths::get);
 				} catch (InvalidPathException e) {
 					throw new AgentOptionParseException("Invalid path given for option 'upload-metadata'", e);
 				}
 				return true;
 			case "duplicates":
 				options.duplicateClassFileBehavior = parseEnumValue(key, value, EDuplicateClassFileBehavior.class);
 				return true;
 			case "ignore-uncovered-classes":
 				options.ignoreUncoveredClasses = Boolean.parseBoolean(value);
 				return true;
 			case "obfuscate-security-related-outputs":
 				options.obfuscateSecurityRelatedOutputs = Boolean.parseBoolean(value);
 				return true;
 			case "dump-on-exit":
 				options.shouldDumpOnExit = Boolean.parseBoolean(value);
 				return true;
 			case "search-git-properties-recursively":
 				options.searchGitPropertiesRecursively = Boolean.parseBoolean(value);
 				return true;
 			case ARTIFACTORY_GIT_PROPERTIES_JAR_OPTION:
 				logger.warn(
 						"The option " + ARTIFACTORY_GIT_PROPERTIES_JAR_OPTION + " is deprecated. It still has an effect, " +
 								"but should be replaced with the equivalent option " + AgentOptions.GIT_PROPERTIES_JAR_OPTION + ".");
 				// intended fallthrough (acts as alias)
 			case AgentOptions.GIT_PROPERTIES_JAR_OPTION:
 				options.gitPropertiesJar = getGitPropertiesJarFile(value);
 				return true;
 			case ARTIFACTORY_GIT_PROPERTIES_COMMIT_DATE_FORMAT_OPTION:
 				logger.warn(
 						"The option " + ARTIFACTORY_GIT_PROPERTIES_COMMIT_DATE_FORMAT_OPTION + " is deprecated. It still has an effect, " +
 								"but should be replaced with the equivalent option " + AgentOptions.GIT_PROPERTIES_COMMIT_DATE_FORMAT_OPTION + ".");
 				// intended fallthrough (acts as alias)
 			case AgentOptions.GIT_PROPERTIES_COMMIT_DATE_FORMAT_OPTION:
 				options.gitPropertiesCommitTimeFormat = DateTimeFormatter.ofPattern(value);
 				return true;
 			case "mode":
 				options.mode = parseEnumValue(key, value, EMode.class);
 				return true;
 			case "includes":
 				options.jacocoIncludes = value.replaceAll(";", ":");
 				return true;
 			case "excludes":
 				options.jacocoExcludes = value.replaceAll(";", ":") + ":" + AgentOptions.DEFAULT_EXCLUDES;
 				return true;
 			case "class-dir":
 				List<String> list = splitMultiOptionValue(value);
 				try {
 					options.classDirectoriesOrZips = ClasspathUtils.resolveClasspathTextFiles(key, filePatternResolver,
 							list);
 				} catch (IOException e) {
 					throw new AgentOptionParseException(e);
 				}
 				return true;
 			case "http-server-port":
 				options.httpServerPort = parseInt(key, value);
 				return true;
 			case "sap-nwdi-applications":
 				options.sapNetWeaverJavaApplications = SapNwdiApplication.parseApplications(value);
 				return true;
 			case "tia-mode":
 				options.testwiseCoverageMode = AgentOptionsParser.parseEnumValue(key, value,
 						ETestwiseCoverageMode.class);
 				return true;
 			default:
 				return false;
 		}
 	}
 
-	private void storeConfigId(AgentOptions options, String configId) throws AgentOptionParseException {
-		if (!options.teamscaleServer.isConfiguredForServerConnection()) {
-			throw new AgentOptionParseException(
-					"Has specified config-id '" + configId + "' without teamscale url/user/accessKey! The options need to be defined in teamscale.properties.");
-		}
-		options.teamscaleServer.configId = configId;
-	}
-
 	private void readConfigFromTeamscale(
 			AgentOptions options) throws AgentOptionParseException, AgentOptionReceiveException {
 		if (options.teamscaleServer.configId == null) {
 			return;
 		}
-
+		if (!options.teamscaleServer.isConfiguredForServerConnection()) {
+			throw new AgentOptionParseException(
+					"Config-id '" + options.teamscaleServer.configId + "' specified without teamscale url/user/accessKey! These options must be provided locally via config-file or command line argument.");
+		}
+		// Set ssl validation option in case it needs to be off before trying to reach Teamscale.
+		HttpUtils.setShouldValidateSsl(options.shouldValidateSsl());
 		ConfigurationViaTeamscale configuration = ConfigurationViaTeamscale.retrieve(logger,
 				options.teamscaleServer.configId,
 				options.teamscaleServer.url,
 				options.teamscaleServer.userName,
 				options.teamscaleServer.userAccessToken);
 		options.configurationViaTeamscale = configuration;
 		logger.debug(
 				"Received the following options from Teamscale: " + configuration.getProfilerConfiguration().configurationOptions);
 		readConfigFromString(options, configuration.getProfilerConfiguration().configurationOptions);
 	}
 
 	private File getGitPropertiesJarFile(String path) {
 		File jarFile = new File(path);

agent/src/test/java/com/teamscale/jacoco/agent/options/AgentOptionsParserTest.java

@@ -194,6 +194,19 @@ public void sapNwdiRequiresAllTeamscaleOptionsExceptProject() {
 				.hasMessageNotContaining("the 'teamscale-' upload options are incomplete");
 	}
 
+	/**
+	 * Test that we can define a config id first, before adding teamscale server credentials. We still expect an
+	 * exception to be thrown, because there is no Teamscale server to reach, but no parse exception.
+	 */
+	@Test
+	public void testConfigIdOptionOrderIrrelevant() {
+		assertThatThrownBy(() -> parseAndThrow(
+				"config-id=myConfig,teamscale-server-url=http://awesome-teamscale.com,teamscale-user=user,teamscale-access-token=mycoolkey")).hasMessageNotContaining(
+						"Failed to parse agent options")
+				.hasMessageContaining(
+						"Failed to retrieve profiler configuration from Teamscale!");
+	}
+
 	@Test
 	public void revisionOrCommitRequireProject() {
 		assertThatThrownBy(