Clarify and cleanup deprecations

agent-common/src/main/java/com/thoughtworks/go/agent/common/launcher/AgentProcessParent.java

@@ -21,7 +21,7 @@
 
 public interface AgentProcessParent {
     int run(
-            @Deprecated //launcherVersion is unused, but keeping it around because the bootstrapper has this contract with the agent launcher
+            @Deprecated(since = "launcherVersion is unused, but keeping it around because the bootstrapper has this contract with the agent launcher")
                     String launcherVersion,
             String launcherMd5, ServerUrlGenerator urlGenerator, Map<String, String> env, Map<String, String> context);
 }

agent-launcher/src/test/java/com/thoughtworks/go/agent/launcher/LockfileTest.java

@@ -120,7 +120,7 @@ public void shouldReturnTrueIfCanSetLockAndDeleteLockFileWhenDeleteIsCalled() {
     }
 
     @Test
-    public void shouldNotDeleteLockFileIfTryLockHasFailed() throws IOException {
+    public void shouldNotDeleteLockFileIfTryLockDidntWork() throws IOException {
         FileUtils.touch(LOCK_FILE);
         Lockfile lockfile = new Lockfile(LOCK_FILE);
         assertThat(lockfile.tryLock(), is(false));

agent/src/test/java/com/thoughtworks/go/agent/AgentPluginsInitializerIntegrationTest.java

@@ -77,8 +77,8 @@ void shouldRemoveExistingBundledPluginsBeforeInitializingNewPlugins() throws Exc
 
         agentPluginsInitializer.onApplicationEvent(null);
 
-        assertThat(existingBundledPlugin.exists()).isFalse();
-        assertThat(new File(directoryForUnzippedPlugins, "bundled/new-plugin-1.jar").exists()).isTrue();
+        assertThat(existingBundledPlugin).doesNotExist();
+        assertThat(new File(directoryForUnzippedPlugins, "bundled/new-plugin-1.jar")).exists();
     }
 
     @Test
@@ -90,7 +90,7 @@ void shouldReplaceExistingBundledPluginsWithNewPluginsOfSameName() throws Except
 
         agentPluginsInitializer.onApplicationEvent(null);
 
-        assertThat(bundledPlugin.exists()).isTrue();
+        assertThat(bundledPlugin).exists();
         assertThat(FileUtils.readFileToString(bundledPlugin, UTF_8)).isEqualTo("SOME-NEW-CONTENT");
     }
 
@@ -103,8 +103,8 @@ void shouldRemoveExistingExternalPluginsBeforeInitializingNewPlugins() throws Ex
 
         agentPluginsInitializer.onApplicationEvent(null);
 
-        assertThat(existingExternalPlugin.exists()).isFalse();
-        assertThat(new File(directoryForUnzippedPlugins, "external/new-plugin-1.jar").exists()).isTrue();
+        assertThat(existingExternalPlugin).doesNotExist();
+        assertThat(new File(directoryForUnzippedPlugins, "external/new-plugin-1.jar")).exists();
     }
 
     @Test
@@ -116,7 +116,7 @@ void shouldReplaceExistingExternalPluginsWithNewPluginsOfSameName() throws Excep
 
         agentPluginsInitializer.onApplicationEvent(null);
 
-        assertThat(externalPlugin.exists()).isTrue();
+        assertThat(externalPlugin).exists();
         assertThat(FileUtils.readFileToString(externalPlugin, UTF_8)).isEqualTo("SOME-NEW-CONTENT");
     }
 
@@ -129,7 +129,7 @@ void shouldRemoveAnExistingPluginWhenItHasBeenRemovedFromTheServerSide() throws
 
         agentPluginsInitializer.onApplicationEvent(null);
 
-        assertThat(existingExternalPlugin.exists()).isFalse();
+        assertThat(existingExternalPlugin).doesNotExist();
     }
 
     private File setupUnzippedPluginsDirectoryStructure() throws IOException {

agent/src/test/java/com/thoughtworks/go/agent/JobRunnerTest.java

@@ -44,7 +44,7 @@ void setUp() {
         work = mock(Work.class);
         agentIdentifier = new AgentIdentifier("localhost", "127.0.0.1", "uuid");
 
-        new SystemEnvironment().setProperty("serviceUrl", SERVER_URL);
+        new SystemEnvironment().setProperty(SystemEnvironment.SERVICE_URL, SERVER_URL);
         resolver = mock(UpstreamPipelineResolver.class);
     }
 

api/api-artifact-config-v1/src/test/groovy/com/thoughtworks/go/apiv1/artifactconfig/represernter/ArtifactConfigRepresenterTest.groovy

@@ -16,11 +16,7 @@
 package com.thoughtworks.go.apiv1.artifactconfig.represernter
 
 import com.thoughtworks.go.api.util.GsonTransformer
-import com.thoughtworks.go.config.ArtifactConfig
-import com.thoughtworks.go.config.ArtifactDirectory
-import com.thoughtworks.go.config.PurgeSettings
-import com.thoughtworks.go.config.PurgeStart
-import com.thoughtworks.go.config.PurgeUpto
+import com.thoughtworks.go.config.*
 import org.junit.jupiter.api.Test
 
 import static com.thoughtworks.go.CurrentGoCDVersion.apiDocsUrl
@@ -129,8 +125,8 @@ class ArtifactConfigRepresenterTest {
       ],
       "artifacts_dir" : "",
       "purge_settings": [
-        "purge_start_disk_space": new Double(20.0),
-        "purge_upto_disk_space" : new Double(10.0),
+        "purge_start_disk_space": Double.valueOf(20.0),
+        "purge_upto_disk_space" : Double.valueOf(10.0),
         "errors"                : [
           "purge_start_disk_space": ["purge-start-error"],
           "purge_upto_disk_space" : ["purge-upto-error"]

api/api-base/src/main/java/com/thoughtworks/go/api/representers/EnvironmentVariableRepresenter.java

@@ -66,7 +66,7 @@ public static EnvironmentVariableConfig fromJSON(JsonReader jsonReader) {
         Optional<String> optValue = jsonReader.optString("value");
         Optional<String> optEncValue = jsonReader.optString("encrypted_value");
 
-        if (!optValue.isPresent() && !optEncValue.isPresent()) {
+        if (optValue.isEmpty() && optEncValue.isEmpty()) {
             HaltApiResponses.haltBecauseOfReason("Environment variable must contain either 'value' or 'encrypted_value'");
         }
 

api/api-config-repos-v4/src/main/java/com/thoughtworks/go/apiv4/configrepos/representers/RulesRepresenter.java

@@ -66,7 +66,7 @@ private static Directive getDirective(JsonElement directiveJson) {
 
         Optional<DirectiveType> directiveType = fromString(directive);
 
-        if (!directiveType.isPresent()) {
+        if (directiveType.isEmpty()) {
             return new Unknown(directive, action, type, resource);
         }
 

api/api-internal-environments-v1/src/main/java/com/thoughtworks/go/apiv1/internalenvironments/representers/EnvironmentAgentRepresenter.java

@@ -18,18 +18,17 @@
 import com.thoughtworks.go.api.base.OutputWriter;
 import com.thoughtworks.go.config.EnvironmentAgentConfig;
 import com.thoughtworks.go.config.EnvironmentConfig;
-import com.thoughtworks.go.config.merge.MergeEnvironmentConfig;
 import com.thoughtworks.go.config.remote.ConfigOrigin;
 import com.thoughtworks.go.config.remote.FileConfigOrigin;
 
 class EnvironmentAgentRepresenter {
-    private static FileConfigOrigin FILE_CONFIG_ORIGIN = new FileConfigOrigin();
+    private static final FileConfigOrigin FILE_CONFIG_ORIGIN = new FileConfigOrigin();
 
     static void toJSON(OutputWriter writer, EnvironmentAgentConfig agent, EnvironmentConfig environmentConfig) {
         writer.add("uuid", agent.getUuid());
         ConfigOrigin origin = environmentConfig.isLocal()
                 ? FILE_CONFIG_ORIGIN
-                : ((MergeEnvironmentConfig) environmentConfig).getOriginForAgent(agent.getUuid());
+                : environmentConfig.originForAgent(agent.getUuid()).orElse(null);
         writer.addChild("origin", originWriter -> EntityConfigOriginRepresenter.toJSON(originWriter, origin));
     }
 }

api/api-internal-environments-v1/src/test/groovy/com/thoughtworks/go/apiv1/internalenvironments/representers/EnvironmentAgentRepresenterTest.groovy

@@ -64,7 +64,7 @@ class EnvironmentAgentRepresenterTest {
     def origin = new RepoConfigOrigin(ConfigRepoConfig.createConfigRepoConfig(MaterialConfigsMother.git("foo.git"), "json-plugon", "repo1"), "revision1");
     def agent = new EnvironmentAgentConfig("agent-1")
     when(environmentConfig.isLocal()).thenReturn(false)
-    when(environmentConfig.getOriginForAgent(agent.uuid)).thenReturn(origin)
+    when(environmentConfig.originForAgent(agent.uuid)).thenReturn(Optional.of(origin))
     def actualJSON = toObjectString({ EnvironmentAgentRepresenter.toJSON(it, agent, environmentConfig) })
 
     def expectedJSON = [

api/api-roles-config-v3/src/main/java/com/thoughtworks/go/apiv3/rolesconfig/representers/DirectiveRepresenter.java

@@ -52,7 +52,7 @@ public static Directive fromJSON(JsonReader reader) {
 
         Optional<DirectiveType> directiveType = fromString(directive);
 
-        if (!directiveType.isPresent()) {
+        if (directiveType.isEmpty()) {
             return new Unknown(directive, action, type, resource);
         }
 

api/api-secret-configs-v3/src/main/java/com/thoughtworks/go/apiv3/secretconfigs/representers/RulesRepresenter.java

@@ -66,7 +66,7 @@ private static Directive getDirective(JsonElement directiveJson) {
 
         Optional<DirectiveType> directiveType = fromString(directive);
 
-        if (!directiveType.isPresent()) {
+        if (directiveType.isEmpty()) {
             return new Unknown(directive, action, type, resource);
         }
 

api/api-stage-instance-v3/src/test/groovy/com/thoughtworks/go/apiv3/stageinstance/StageInstanceControllerV3Test.groovy

@@ -47,6 +47,7 @@ import org.mockito.junit.jupiter.MockitoSettings
 import org.mockito.quality.Strictness
 
 import java.sql.Timestamp
+import java.time.LocalDateTime
 import java.util.stream.Stream
 
 import static com.thoughtworks.go.api.base.JsonUtils.toObjectString
@@ -69,7 +70,7 @@ class StageInstanceControllerV3Test implements SecurityServiceTrait, ControllerT
   }
 
   @Nested
-  class RerunFailedJobs {
+  class RerunBadJobs {
     @Nested
     class Security implements SecurityTestTrait, PipelineGroupOperateUserSecurity {
 
@@ -98,13 +99,13 @@ class StageInstanceControllerV3Test implements SecurityServiceTrait, ControllerT
       }
 
       @Test
-      void 'reruns failed-jobs for a stage'() {
+      void 'reruns bad-jobs for a stage'() {
         Stage stage = mock(Stage)
         String expectedResponseBody = "Request to rerun job(s) is accepted"
 
         when(stageService.findStageWithIdentifier(eq("up42"), eq(3), eq("stage1"), eq("1"), anyString(), any() as HttpOperationResult)).thenReturn(stage)
         when(scheduleService.rerunFailedJobs(any() as Stage, any() as HttpOperationResult)).then({ InvocationOnMock invocation ->
-          HttpOperationResult operationResult = invocation.getArguments().last()
+          HttpOperationResult operationResult = invocation.getArguments().last() as HttpOperationResult
           operationResult.accepted(expectedResponseBody, "", HealthStateType.general(HealthStateScope.forStage("up42", "stage1")))
           return stage
         })
@@ -196,7 +197,7 @@ class StageInstanceControllerV3Test implements SecurityServiceTrait, ControllerT
         when(stageService.findStageWithIdentifier(eq("up42"), eq(3), eq("stage1"), eq("1"), anyString(), any() as HttpOperationResult)).thenReturn(this.stage)
         when(scheduleService.rerunJobs(eq(this.stage), eq(jobs), any() as HttpOperationResult))
           .then({ InvocationOnMock invocation ->
-          HttpOperationResult result = invocation.getArguments().last()
+          HttpOperationResult result = invocation.getArguments().last() as HttpOperationResult
           result.accepted(expectedMessage, "", HealthStateType.general(HealthStateScope.forStage("up42", "stage1")))
           return this.stage
         })
@@ -435,9 +436,9 @@ class StageInstanceControllerV3Test implements SecurityServiceTrait, ControllerT
         jobInstance.setState(JobState.Assigned)
         jobInstance.setResult(JobResult.Unknown)
         jobInstance.setAgentUuid("uuid")
-        jobInstance.setScheduledDate(new Date(2018, 12, 21, 12, 30))
+        jobInstance.setScheduledDate(LocalDateTime.of(2018, 12, 21, 12, 30).toDate())
         jobInstance.setOriginalJobId(1)
-        jobInstance.setTransitions(new JobStateTransitions(new JobStateTransition(JobState.Scheduled, new Date(2018, 12, 21, 12, 45)),
+        jobInstance.setTransitions(new JobStateTransitions(new JobStateTransition(JobState.Scheduled, LocalDateTime.of(2018, 12, 21, 12, 45).toDate()),
           new JobStateTransition(JobState.Assigned, null)))
 
         return jobInstance
@@ -611,7 +612,7 @@ class StageInstanceControllerV3Test implements SecurityServiceTrait, ControllerT
       }
 
       def getStageModels() {
-        def jobHistoryItem = new JobHistoryItem("job", JobState.Completed, JobResult.Passed, new Date(2018, 12, 22, 11, 10))
+        def jobHistoryItem = new JobHistoryItem("job", JobState.Completed, JobResult.Passed, LocalDateTime.of(2018, 12, 22, 11, 10).toDate())
         jobHistoryItem.setId(34)
         def jobHistory = new JobHistory()
         jobHistory.add(jobHistoryItem)

api/api-stage-operations-v2/src/main/java/com/thoughtworks/go/apiv2/stageoperations/StageOperationsControllerV2.java

@@ -78,7 +78,7 @@ public String triggerStage(Request req, Response res) throws IOException {
         HttpOperationResult result = new HttpOperationResult();
 
         Optional<Integer> pipelineCounterValue = pipelineService.resolvePipelineCounter(pipelineName, pipelineCounter);
-        if (!pipelineCounterValue.isPresent()) {
+        if (pipelineCounterValue.isEmpty()) {
             String errorMessage = String.format("Error while running [%s/%s/%s]. Received non-numeric pipeline counter '%s'.", pipelineName, pipelineCounter, stageName, pipelineCounter);
             LOGGER.error(errorMessage);
             throw haltBecauseOfReason(errorMessage);

base/src/main/java/com/thoughtworks/go/util/SystemEnvironment.java

@@ -314,20 +314,6 @@ public File getJettyConfigFile() {
         return new File(getConfigDir(), get(JETTY_XML_FILE_NAME));
     }
 
-    /**
-     * @deprecated use <code>new SystemEnvironment().getXXXXX()</code> instead.
-     */
-    public static String getProperty(String property, String defaultValue) {
-        return new SystemEnvironment().getPropertyImpl(property, defaultValue);
-    }
-
-    /**
-     * @deprecated use <code>new SystemEnvironment().getXXXXX()</code> instead.
-     */
-    public static String getProperty(String property) {
-        return new SystemEnvironment().getPropertyImpl(property);
-    }
-
     private String getPropertyImpl(String property, String defaultValue) {
         return System.getProperty(property, defaultValue);
     }

common/src/main/java/com/thoughtworks/go/domain/User.java

@@ -237,7 +237,7 @@ public void validateMatcher() throws ValidationException {
 
     public void validateEmail() throws ValidationException {
         validate(Validator.lengthValidator(255), getEmail());
-        validate(Validator.EMAIL, getEmail());
+        validate(Validator.emailValidator(), getEmail());
     }
 
     public void validateLoginName() throws ValidationException {

common/src/main/java/com/thoughtworks/go/presentation/pipelinehistory/PipelineInstanceModel.java

@@ -341,9 +341,7 @@ public String getPipelineStatusMessage() {
         return String.format("%s: %s", latestStage.getState(), latestStage.getName());
     }
 
-    /**
-     * @deprecated use the other construction methods
-     */
+    @Deprecated(since = "use the other construction methods")
     public static PipelineInstanceModel createEmptyModel() {
         return new PipelineInstanceModel();
     }

common/src/main/java/com/thoughtworks/go/presentation/pipelinehistory/StageHistoryEntry.java

@@ -90,11 +90,7 @@ public boolean equals(Object o) {
         if (state != that.state) {
             return false;
         }
-        if (rerunOfCounter != null ? !rerunOfCounter.equals(that.rerunOfCounter) : that.rerunOfCounter != null) {
-            return false;
-        }
-
-        return true;
+        return rerunOfCounter != null ? rerunOfCounter.equals(that.rerunOfCounter) : that.rerunOfCounter == null;
     }
 
     @Override
@@ -125,7 +121,7 @@ public boolean hasRerunJobs() {
         return rerunOfCounter != null;
     }
 
-    @Deprecated
+    @Deprecated(since = "Only for DB fw usage")
     public void setRerunOfCounter(Integer rerunOfCounter) {
         this.rerunOfCounter = rerunOfCounter;
     }

common/src/main/java/com/thoughtworks/go/server/service/result/OperationResult.java

@@ -23,16 +23,12 @@
  */
 public interface OperationResult {
 
-    @Deprecated
     ServerHealthState success(HealthStateType healthStateType);
 
-    @Deprecated
     ServerHealthState error(String message, String description, HealthStateType type);
 
-    @Deprecated
     ServerHealthState warning(String message, String description, HealthStateType type);
 
-    @Deprecated
     ServerHealthState getServerHealthState();
 
     boolean canContinue();

common/src/test/java/com/thoughtworks/go/agent/URLServiceTest.java

@@ -39,7 +39,7 @@ public void setUp() {
 
     @AfterEach
     public void teardown() {
-        new SystemEnvironment().clearProperty("serviceUrl");
+        new SystemEnvironment().clearProperty(SystemEnvironment.SERVICE_URL);
     }
 
     @Test
@@ -68,7 +68,7 @@ public void shouldReturnRestfulUrlOfAgentWithAttemptCounter() {
 
     @Test
     public void shouldReturnServerUrlWithSubpath() {
-        new SystemEnvironment().setProperty("serviceUrl", BASE_URL + "/");
+        new SystemEnvironment().setProperty(SystemEnvironment.SERVICE_URL, BASE_URL + "/");
         assertThat(new URLService().serverUrlFor("someSubPath/xyz"), is(BASE_URL + "/someSubPath/xyz"));
     }
 

common/src/test/java/com/thoughtworks/go/agent/testhelpers/DefaultWorkCreator.java

@@ -28,7 +28,6 @@
 import com.thoughtworks.go.remote.work.BuildAssignment;
 import com.thoughtworks.go.remote.work.BuildWork;
 import com.thoughtworks.go.remote.work.Work;
-import com.thoughtworks.go.util.SystemEnvironment;
 import com.thoughtworks.go.util.TestFileUtil;
 import com.thoughtworks.go.util.command.EnvironmentVariableContext;
 
@@ -60,7 +59,7 @@ private static File tempFile() {
     }
 
     private static File tempFolder() {
-        File tmpdir = new File(SystemEnvironment.getProperty("java.io.tmpdir"), "goServerStub");
+        File tmpdir = new File(System.getProperty("java.io.tmpdir"), "goServerStub");
         tmpdir.deleteOnExit();
         final File dir = new File(tmpdir, "testdir");
         if (dir.exists() && dir.isDirectory()) {
@@ -103,6 +102,6 @@ public Work work(AgentIdentifier agentIdentifier) {
 
     private JobPlan toPlan(final CruiseConfig config) {
         JobConfig plan = config.jobConfigByName(PIPELINE_NAME, STAGE_NAME, JOB_PLAN_NAME, true);
-        return JobInstanceMother.createJobPlan(plan, new JobIdentifier(PIPELINE_NAME, PIPELINE_LABEL, STAGE_NAME, "1", JOB_PLAN_NAME), new DefaultSchedulingContext());
+        return JobInstanceMother.createJobPlan(plan, new JobIdentifier(PIPELINE_NAME, 1, PIPELINE_LABEL, STAGE_NAME, "1", JOB_PLAN_NAME), new DefaultSchedulingContext());
     }
 }

common/src/test/java/com/thoughtworks/go/agent/testhelpers/FakeBuildRepositoryRemote.java

@@ -23,7 +23,6 @@
 import com.thoughtworks.go.remote.BuildRepositoryRemote;
 import com.thoughtworks.go.remote.work.Work;
 import com.thoughtworks.go.server.service.AgentRuntimeInfo;
-import com.thoughtworks.go.util.SystemEnvironment;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -53,15 +52,7 @@ public AgentInstruction ping(AgentRuntimeInfo info) {
 
     @Override
     public Work getWork(AgentRuntimeInfo runtimeInfo) {
-        String className = SystemEnvironment.getProperty("WORKCREATOR", DefaultWorkCreator.class.getCanonicalName());
-        Class<? extends WorkCreator> aClass = null;
-        try {
-            aClass = (Class<? extends WorkCreator>) Class.forName(className);
-            return aClass.getDeclaredConstructor().newInstance().work(runtimeInfo.getIdentifier());
-        } catch (Exception e) {
-            e.printStackTrace();
-            throw new RuntimeException(e);
-        }
+        return new DefaultWorkCreator().work(runtimeInfo.getIdentifier());
     }
 
     @Override