Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

Replace build time environment with runtime environment #1099

Merged
merged 3 commits into from
Jan 29, 2020

Conversation

Jokinen
Copy link
Contributor

@Jokinen Jokinen commented Jan 28, 2020

By using a runtime environment, the app doesn't need to be rebuild on
environment changes.

This change also fixes an issue we had with the test environment. The
test environment only sets the environment variables for runtime. This
means that we don't have access to environment variables during build
time, which we assumed to be the case previously.

Ref: VAR-235

@Jokinen
Copy link
Contributor Author

Jokinen commented Jan 28, 2020

I accidentally required CUSTOM_MUNCIPALITY_OPTIONS by directly parsing it with JSON.parse. I reverted this change.

@codecov-io
Copy link

codecov-io commented Jan 28, 2020

Codecov Report

Merging #1099 into develop will increase coverage by 0.02%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1099      +/-   ##
===========================================
+ Coverage    72.32%   72.34%   +0.02%     
===========================================
  Files          253      253              
  Lines         3801     3804       +3     
  Branches       675      675              
===========================================
+ Hits          2749     2752       +3     
  Misses         904      904              
  Partials       148      148
Impacted Files Coverage Δ
...servation-information/InternalReservationFields.js 40% <ø> (ø) ⬆️
...es/reservation/reservation-information/Textarea.js 50% <ø> (ø) ⬆️
src/index.js 0% <0%> (ø) ⬆️
src/common/calendar/TimePickerCalendar.js 25% <100%> (ø) ⬆️
app/utils/testUtils.js 89.87% <100%> (+0.39%) ⬆️
app/i18n/initI18n.js 100% <100%> (ø) ⬆️
src/domain/search/utils.js 87.8% <50%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5de7193...a595e57. Read the comment docs.

Copy link

@badrange badrange left a comment

Choose a reason for hiding this comment

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

Please check comma-dangling and ensure that you're only making intended changes!

@Jokinen
Copy link
Contributor Author

Jokinen commented Jan 28, 2020

I accidentally used an es6 export mixed in with module.exports, fixed that now.

This enforces the use of comma dangle.
@Jokinen Jokinen force-pushed the fix/test-environment branch from 7e4803f to 7cd4726 Compare January 28, 2020 10:50
@Jokinen
Copy link
Contributor Author

Jokinen commented Jan 28, 2020

I removed the custom eslint rule that disabled the comma dangle rules set in airbnb's package. I ran fix which added the dangling comma to all the places it was missing from.

I cleaned the commit history on the same go.

By using a runtime environment, the app doesn't need to be rebuild on
environment changes.

This change also fixes an issue we had with the test environment. The
test environment only sets the environment variables for runtime. This
means that we don't have access to environment variables during build
time, which we assumed to be the case previously.
@Jokinen Jokinen force-pushed the fix/test-environment branch from 7cd4726 to a595e57 Compare January 28, 2020 10:58
Copy link
Contributor

@tuovinensanttu tuovinensanttu 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 and everything seems to be working, well done.

Copy link

@badrange badrange left a comment

Choose a reason for hiding this comment

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

Good!

@Jokinen Jokinen merged commit 10899f5 into develop Jan 29, 2020
@Jokinen Jokinen deleted the fix/test-environment branch January 29, 2020 07:05
@Jokinen Jokinen mentioned this pull request Feb 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants