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

chore: release on tag #4208

Merged
merged 1 commit into from
Aug 1, 2023
Merged

chore: release on tag #4208

merged 1 commit into from
Aug 1, 2023

Conversation

drewbo
Copy link
Contributor

@drewbo drewbo commented Jul 27, 2023

Changes proposed in this pull request:

  • This PR removes the git-branch and product instance variables and explicitly defines three different CI pipelines for this repo:
    • pipeline-dev.yml: largely the same as before with modularized task definitions (added to ci/partials). The yarn build-dev command was removed in favor of using NODE_ENV to define the build changes and that same env variable is passed to the admin client build as well.
    • pipeline-staging.yml: the same as before but with modularized task definitions from above. The deploy-env is removed from the names of PR tests here because they are now deploy env independent.
    • pipeline-production.yml: removes the testing of PRs (since all PRs to main are tested in the prior pipeline). Incorporates the release PR management which was previously added as a separate pipeline

security considerations

None

@drewbo drewbo force-pushed the chore-release-on-tag branch 2 times, most recently from d981cff to 4b6ac55 Compare July 31, 2023 16:48
@drewbo drewbo requested a review from a team July 31, 2023 17:25
@@ -163,7 +163,7 @@ To deploy to CloudFoundry submit the following:
`cf push federalistapp --strategy rolling --vars-file "./.cloudgov/vars/${CF_SPACE}.yml" -f ./cloudgov/manifest.yml`

### Continuous Integration
We are in the process of migrating from CircleCI to an internal instance of Concourse CI, starting with our staging environment. To use Concourse, one must have appropriate permissions in UAA as administered by the cloud.gov operators. Access to Concourse also requires using the GSA VPN.
Our continuous integration pipeline is run on Concourse CI. To use Concourse, one must have appropriate permissions in UAA as administered by the cloud.gov operators. Access to Concourse also requires using the GSA VPN.
Copy link
Contributor

Choose a reason for hiding this comment

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

Access to Concourse also requires using the GSA VPN.

Maybe it's beyond the scope of this documentation, but Zscaler changes this, or is at least in the process of doing so.

Copy link
Contributor

@svenaas svenaas left a comment

Choose a reason for hiding this comment

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

Made some copyediting suggestions.

DEVELOPMENT.md Outdated Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
@@ -202,7 +205,7 @@ Some credentials in this pipeline are "compound" credentials that use the pipeli
--- | --- | --- |
|**`((deploy-env))-cf-username`**|The deployment environments CloudFoundry deployer username based on the instanced pipeline|:white_check_mark:|
Copy link
Contributor

Choose a reason for hiding this comment

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

Either "development environment's" or "development environment".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Skipping these because they aren't part of this PR (will catch on a larger docs review)

@@ -202,7 +205,7 @@ Some credentials in this pipeline are "compound" credentials that use the pipeli
--- | --- | --- |
|**`((deploy-env))-cf-username`**|The deployment environments CloudFoundry deployer username based on the instanced pipeline|:white_check_mark:|
|**`((deploy-env))-cf-username`**|The deployment environments CloudFoundry deployer password based on the instanced pipeline|:white_check_mark:|
Copy link
Contributor

Choose a reason for hiding this comment

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

Either "development environment's" or "development environment".

@apburnes apburnes self-requested a review August 1, 2023 14:48
Copy link
Contributor

@apburnes apburnes left a comment

Choose a reason for hiding this comment

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

Looks really good and much easier to read. The partials should work out well. Added a couple of comments and questions for clarification. Overall all a great step in the right direction.

ci/pipeline-dev.yml Show resolved Hide resolved
ci/pipeline-production.yml Outdated Show resolved Hide resolved
ci/pipeline-production.yml Outdated Show resolved Hide resolved
ci/pipeline-production.yml Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this update related to CI tasks or clean up you came across?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's related: we previously had two commands yarn build and yarn build-dev with slightly different webpack configs. This was introduced to support the dev build options like nicer error tracing. To modularize the task, I wanted to share the same command (now encapsulated as a task in ci/partials/build-api.yml) and webpack config file but configure the differences with an environment variable (NODE_ENV).

Separately, I think it would be even better practice to combine development and production webpack configs into one file and make it even clearer how environment variables swap between different options but I decided that was out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is of immediate interest to me because of #4191. I noticed that build-dev exists but isn't utilized in test:prepare. Also we have both webpack.development-build.config.js (used by build-dev) and webpack.development.config.js (not referenced at all in package.json).

Copy link
Contributor

Choose a reason for hiding this comment

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

For #4191 I've been considering whether we might need a separate webpack config when building for local testing in order to have a different output folder from what we use on the hosted instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@svenaas build-dev is specifically for the concourse dev deploy. webpack.development.config.js is used during our local build (but is included via middleware, not via --config in the CLI)

Copy link
Contributor

Choose a reason for hiding this comment

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

@drewbo thanks for the clarification. Let me know if we want to flesh out an issue to combine the webpack configs.

@drewbo drewbo changed the base branch from staging to main August 1, 2023 16:14
@apburnes apburnes self-requested a review August 1, 2023 19:20
Copy link
Contributor

@apburnes apburnes left a comment

Choose a reason for hiding this comment

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

There's a lot here but the structure solid and dev ci is working as expected. Thanks for handling this work!

@drewbo drewbo changed the base branch from main to staging August 1, 2023 19:48
@drewbo drewbo changed the base branch from staging to main August 1, 2023 19:49
@drewbo drewbo merged commit 3283259 into main Aug 1, 2023
3 checks passed
@drewbo drewbo deleted the chore-release-on-tag branch August 1, 2023 20:08
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.

4 participants