-
Notifications
You must be signed in to change notification settings - Fork 9
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
#179 update ci profile to use fabric8 for multi-platform docker build #187
Conversation
@@ -45,21 +45,12 @@ public class HashingSteps { | |||
private static final Logger logger = LoggerFactory.getLogger(HashingSteps.class); | |||
private final GenericContainer<?> genericContainer = new GenericContainer<>( | |||
DockerImageName.parse( | |||
"ghcr.io/boozallen/aissemble-vault:" + System.getProperty("version.aissemble.vault") + this.getSystemArchitectureTag() | |||
"ghcr.io/boozallen/aissemble-vault:" + System.getProperty("version.aissemble.vault") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I: remove the system architecture from the tag
baf1da4
to
dc8aa89
Compare
@@ -3,7 +3,7 @@ services: | |||
vault: | |||
# For testing we will pull the image directly from the remote server as opposed to a locally built image. | |||
# This will ensure that the Vault keys are in sync. | |||
image: "ghcr.io/boozallen/aissemble-vault:${version.aissemble}-amd64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I: remove the system architecture from the tag
@@ -75,7 +68,7 @@ def before_feature(context, feature): | |||
f"Starting services defined in Docker Compose file at {test_staging_path}" | |||
) | |||
|
|||
compose = DockerCompose(test_staging_path, select_docker_compose_arch()) | |||
compose = DockerCompose(test_staging_path, "docker-compose.yml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I: We don't need different docker compose files anymore since we don't need to specify the system architecture for the docker image
</plugins> | ||
</pluginManagement> | ||
</build> | ||
</profile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I: remove system architecture specific profile since fabric8 will build both platforms
f5b09a4
to
ca56138
Compare
default: return "-amd64"; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I: removing the system architecture specific image tag
<executions> | ||
<execution> | ||
<id>default-push</id> | ||
<configuration> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: Instead of duplicating the build_args, we should just move the config block below from the default-build
execution to the plugin root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the execution doesn't recognize the configuration after removing the execution id so will continue with this approach
<SPARK_VERSION>${version.spark}</SPARK_VERSION> | ||
</args> | ||
<contextDir>${project.basedir}</contextDir> | ||
<dockerFile>./src/main/resources/docker/Dockerfile</dockerFile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this configuration needed? Looks the same as the config specified in the build-parent/pom.xml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is <id>default-push</id>
instead of <id>default-build</id>
, so makes since to have to duplicate them.
ca56138
to
af247b7
Compare
No description provided.