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

Modifying engine cart CI build parameterization #4256

Merged
merged 1 commit into from
Feb 4, 2020

Conversation

jeremyf
Copy link
Contributor

@jeremyf jeremyf commented Feb 4, 2020

At present, EngineCart's parsing of .engine_cart.yml will overwrite
the ENGINE_CART_RAILS_OPTIONS set in CircleCI (see
cbeer/engine_cart/102 for some details). The present overwrite
behavior is reasonable. As the removed .engine_cart.yml targeted
pre-Rails 5 behavior, I opted to remove it.

In doing so, this would now "activate" the .circleci/config.yml
declaration of ENGINE_CART_RAILS_OPTIONS. As we have coffee script in
Hyrax (for better or worse) and we have ActionCable usage, I needed to
revisit the now activated parameters.

Note: This is in preparation for allowing PostgreSQL as the database
for CircleCI.

As this impacts the .internal_test_app generation, I also needed to
bump the Regen number.

TL;DR: A 4 year old file that we no longer need is silently trampling
on a CI variable. That CI variable has likely not been used. It now will
be used, but needed modifications to reflect that usage.

I hope this works, as iterating on the CI process is always time
consuming as it requires coordination of remote resources.

@samvera/hyrax-code-reviewers

cjcolvar
cjcolvar previously approved these changes Feb 4, 2020
Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me.

@jeremyf jeremyf force-pushed the removing-engine-cart-template branch 4 times, most recently from 059d926 to f42f550 Compare February 4, 2020 20:15
At present, EngineCart's parsing of `.engine_cart.yml` will overwrite
the `ENGINE_CART_RAILS_OPTIONS` set in CircleCI (see
[cbeer/engine_cart/102][1] for some details). The present overwrite
behavior is reasonable. As the removed `.engine_cart.yml` targeted
pre-Rails 5 behavior, I opted to remove it.

In doing so, this would now "activate" the `.circleci/config.yml`
declaration of `ENGINE_CART_RAILS_OPTIONS`. As we have coffee script in
Hyrax (for better or worse) and we have ActionCable usage, I needed to
revisit the now activated parameters. The parameters to which I changed
are the defaults of EngineCart (as of this PR).

Note: This is in preparation for allowing PostgreSQL as the database
for CircleCI.

As this impacts the `.internal_test_app` generation, I also needed to
bump the Regen number.

TL;DR: A 4 year old file that we no longer need is silently trampling
on a CI variable. That CI variable has likely not been used. It now will
be used, but needed modifications to reflect that usage.

_I hope this works, as iterating on the CI process is always time
consuming as it requires coordination of remote resources._

[1]:cbeer/engine_cart#102
@jeremyf jeremyf force-pushed the removing-engine-cart-template branch from f42f550 to 2d2dcf6 Compare February 4, 2020 21:10
@straleyb
Copy link
Contributor

straleyb commented Feb 4, 2020

@jeremyf When this gets merged, will people have to regenerate their internal_test_apps? Just in case I get to doing some work at some point in time this week.

@jeremyf
Copy link
Contributor Author

jeremyf commented Feb 4, 2020 via email

Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

👍 Looks like you needed to save yarn which makes sense since I think it is being used for including UV now.

@jeremyf jeremyf merged commit c209a88 into master Feb 4, 2020
@jeremyf jeremyf deleted the removing-engine-cart-template branch February 4, 2020 22:36
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.

3 participants