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

BUG: Configuration Store added to Tiltfile as suggested by manual action fails to start #370

Closed
ewilkins-csi opened this issue Sep 26, 2024 · 1 comment · Fixed by #376
Closed
Assignees
Labels
bug Something isn't working slacktask Small ticket done in down time
Milestone

Comments

@ewilkins-csi
Copy link
Contributor

ewilkins-csi commented Sep 26, 2024

Description

We intended to remove the manual action to add the config store to Tilt, but inadvertently just reverted it back to using the standard Tilt deployment action message. Adding the store in this way causes failures as noted in #353 (comment).

Steps to Reproduce

  1. Create a project from 1.9.1
  2. Build the project following manual actions
  3. Tilt up
  4. The config-store-webhook-init job will show as failed

Expected Behavior

The build does not recommend adding the config store to the Tiltfile.

Actual Behavior

The build recommends adding the config store in a way that does not work.

Additional Context

  • Version 1.9.1

Definition of Done

  • Fix bug above so that the expected behavior is implemented
  • Release patch 1.9.2

Test Steps

OTS

  1. Create a project from 1.9.2 (OTS: 1.9.2-SNAPSHOT)
mvn archetype:generate -DarchetypeGroupId=com.boozallen.aissemble \
                       -DarchetypeArtifactId=foundation-archetype \
                       -DarchetypeVersion=1.9.2-SNAPSHOT \
                       -DgroupId=org.test -DartifactId=verify-1-9-2 \
                       -DprojectGitUrl=test.org/verify-1-9-2.git \
                       -DprojectName="Verify 1.9.2 Release" \
&& cd verify-1-9-2
  1. Add the following to verify-1-9-2-deploy/pom.xml with the other Fermenter executions:
                    <execution>
                        <id>configuration-store</id>
                        <phase>generate-sources</phase>
                        <goals>
                            <goal>generate-sources</goal>
                        </goals>
                        <configuration>
                            <basePackage>src/main/</basePackage>
                            <profile>configuration-store-deploy-v2</profile>
                            <propertyVariables>
                                <appName>configuration-store</appName>
                            </propertyVariables>
                        </configuration>
                    </execution>
  1. Run Fermenter and verify no manual action for adding the Configuration Store to Tilt is displayed
mvn clean generate-sources -am -pl :verify-1-9-2-deploy

Final Test

  1. Create a project from 1.9.2 (OTS: 1.9.2-SNAPSHOT)
mvn archetype:generate -DarchetypeGroupId=com.boozallen.aissemble \
                       -DarchetypeArtifactId=foundation-archetype \
                       -DarchetypeVersion=1.9.2\
                       -DgroupId=org.test -DartifactId=verify-1-9-2 \
                       -DprojectGitUrl=test.org/verify-1-9-2.git \
                       -DprojectName="Verify 1.9.2 Release" \
&& cd verify-1-9-2
  1. Run the build until no more manual actions are displayed (You are likely to see a manual action requesting the addition of a pipeline to the project. That manual action can be ignored as it does not impact the testing of this ticket.)
mvn clean install
  1. Run a final install without cache
mvn clean install -Dmaven.build.cache.skipCache
  1. Deploy with tilt up
  2. Verify everything starts and no configuration-store resources are listed
@ewilkins-csi ewilkins-csi added the bug Something isn't working label Sep 26, 2024
@ewilkins-csi ewilkins-csi added this to the 1.9.2 milestone Sep 26, 2024
@ewilkins-csi ewilkins-csi self-assigned this Sep 26, 2024
@ewilkins-csi ewilkins-csi added the slacktask Small ticket done in down time label Sep 26, 2024
ewilkins-csi added a commit that referenced this issue Sep 26, 2024
ewilkins-csi added a commit that referenced this issue Sep 26, 2024
The Configuration Store requires special logic to be deployed properly
via Tilt.  When deciding on where the MVP should be and weighing the
options against our planned move _away_ from Tilt, we decided it was
best to just exclude the configuration store from local deployments for
now and provide projects with instructions upon request.  However, in
doing that we accidentally routed the config store to the standard Tilt
manual action notification, which doesn't work at all.

This change correctly supresses the Tilt manual action if it is for the
config store.  Note that the only real way to ensure this check was to
use the `appName`, which technically could be changed by a downstream
project which would result in the manual action reappearing.  This is an
edge case, and again the plan is to move away from Tilt completely
anyway and that work is scheduled to start immediately after the Java
upgrade.
ewilkins-csi added a commit that referenced this issue Sep 26, 2024
ewilkins-csi added a commit that referenced this issue Sep 26, 2024
The Configuration Store requires special logic to be deployed properly
via Tilt.  When deciding on where the MVP should be and weighing the
options against our planned move _away_ from Tilt, we decided it was
best to just exclude the configuration store from local deployments for
now and provide projects with instructions upon request.  However, in
doing that we accidentally routed the config store to the standard Tilt
manual action notification, which doesn't work at all.

This change correctly supresses the Tilt manual action if it is for the
config store.  Note that the only real way to ensure this check was to
use the `appName`, which technically could be changed by a downstream
project which would result in the manual action reappearing.  This is an
edge case, and again the plan is to move away from Tilt completely
anyway and that work is scheduled to start immediately after the Java
upgrade.
ewilkins-csi added a commit that referenced this issue Sep 26, 2024
The Configuration Store requires special logic to be deployed properly
via Tilt.  When deciding on where the MVP should be and weighing the
options against our planned move _away_ from Tilt, we decided it was
best to just exclude the configuration store from local deployments for
now and provide projects with instructions upon request.  However, in
doing that we accidentally routed the config store to the standard Tilt
manual action notification, which doesn't work at all.

This change correctly supresses the Tilt manual action if it is for the
config store.  Note that the only real way to ensure this check was to
use the `appName`, which technically could be changed by a downstream
project which would result in the manual action reappearing.  This is an
edge case, and again the plan is to move away from Tilt completely
anyway and that work is scheduled to start immediately after the Java
upgrade.
@ewilkins-csi ewilkins-csi linked a pull request Sep 27, 2024 that will close this issue
@ewilkins-csi ewilkins-csi modified the milestones: 1.9.2, 1.10.0 Sep 27, 2024
ewilkins-csi added a commit that referenced this issue Sep 27, 2024
[#370] only show Tilt notification if not config-store
@nartieri
Copy link
Collaborator

Testing Status: Passed! 🎉

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working slacktask Small ticket done in down time
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants