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

#191 archetype and migration update for fabric8 usage in ci and it profiles #203

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

Cho-William
Copy link
Contributor

No description provided.

@Cho-William Cho-William force-pushed the 191-fabric8-profiles branch 2 times, most recently from 3d3c3ba to 8485df8 Compare July 11, 2024 16:48
removeOldConfig.add(pomDeletePreviousVersion);
}

private boolean executeProfileMigration(File file, Plugin profilePlugin, String profileId) {
Copy link
Contributor Author

@Cho-William Cho-William Jul 11, 2024

Choose a reason for hiding this comment

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

this function is the main addition to the migration, the rest of the changes in this file are largely refactorings to make shared helper functions available between the build config and profile migrations

</images>
</configuration>
</execution>
</executions>
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline, but going to move these configs out of the execution block so that it will apply to all executions.

@Cho-William Cho-William force-pushed the 191-fabric8-profiles branch from 0344b2d to af8eb15 Compare July 11, 2024 19:53
<image>
<name>${project.artifactId}:${project.version}</name>
</image>
</images>
Copy link
Contributor

@carter-cundiff carter-cundiff Jul 11, 2024

Choose a reason for hiding this comment

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

Same as above

<name>${project.artifactId}:${project.version}</name>
</image>
</images>
</configuration>
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Why set the image name when the config is set to skip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was brought up at DOD and we as a team need to decide if we want to unify this configuration. Will be covered in different ticket

import com.boozallen.aissemble.upgrade.migration.AbstractAissembleMigration;
import com.boozallen.aissemble.upgrade.util.pom.LocationAwareMavenReader;
import org.apache.maven.model.Build;
import org.apache.maven.model.*;
Copy link
Contributor

@carter-cundiff carter-cundiff Jul 11, 2024

Choose a reason for hiding this comment

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

C: Should avoid using .* on imports.

migratePackaging = true;
}

return migrateConfiguration || migrateProfile || migratePackaging;
Copy link
Contributor

@carter-cundiff carter-cundiff Jul 11, 2024

Choose a reason for hiding this comment

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

S: Could simplify to return orphedomosBuildPlugin != null || orphedomosCiProfilePlugin != null || orphedomosItProfilePlugin != null || containsOrphedomosPackaging(file)

}

/**
* Performs the migration if the shouldExecuteOnFile() returns true
*
Copy link
Contributor

Choose a reason for hiding this comment

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

A: Extra space

@Cho-William Cho-William force-pushed the 191-fabric8-profiles branch 2 times, most recently from 8d3b862 to c31d02a Compare July 11, 2024 20:35
@Cho-William Cho-William force-pushed the 191-fabric8-profiles branch from c31d02a to d340666 Compare July 11, 2024 20:45
@carter-cundiff carter-cundiff merged commit 13e5272 into dev Jul 12, 2024
@carter-cundiff carter-cundiff deleted the 191-fabric8-profiles branch July 12, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants