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

fix(command-build): allow setting context via CONTEXT env, default to production #4040

Merged
merged 4 commits into from Jan 18, 2022

Conversation

erezrokah
Copy link
Contributor

@erezrokah erezrokah commented Jan 14, 2022

πŸŽ‰ Thanks for submitting a pull request! πŸŽ‰

Summary

Fixes #4039

this.name() is always defined, so it overrides the environment variable (which is evaluated in @netlify/config). Also @netlify/config defaults to production context, so we should pass undefined to allow the default behavior.


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code πŸ§‘β€πŸ’». This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire πŸ”₯ (e.g. incident related), you can skip this step.
  • Read the contribution guidelines πŸ“–. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) πŸ§ͺ
  • Update or add documentation (if features were changed or added) πŸ“
  • Make sure the status checks below are successful βœ…

A picture of a cute animal (not mandatory, but encouraged)

@github-actions github-actions bot added the type: bug code to address defects in shipped code label Jan 14, 2022
@github-actions
Copy link

github-actions bot commented Jan 14, 2022

πŸ“Š Benchmark results

Comparing with 41494be

Package size: 355 MB

⬇️ 0.00% decrease vs. 41494be

^  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB         
β”‚   β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”          
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |          
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |          
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |   355 MB 
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    β”Œβ”€β”€β”  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

context:
options.context ||
process.env.CONTEXT ||
(['dev', 'dev:exec', 'dev:trace'].includes(this.name()) ? 'dev' : undefined),
Copy link
Contributor Author

@erezrokah erezrokah Jan 14, 2022

Choose a reason for hiding this comment

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

This was previously done in an oclif child class, not sure if there's a better way to do it now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think either. I would add a small comment here in the code to outline why we are checking for ((['dev', 'dev:exec', 'dev:trace'] to help our future selfs when touching this code again πŸ˜‚

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done in 7ba6367

lukasholzer
lukasholzer previously approved these changes Jan 17, 2022
Copy link
Collaborator

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests and having further safety nets :)

context:
options.context ||
process.env.CONTEXT ||
(['dev', 'dev:exec', 'dev:trace'].includes(this.name()) ? 'dev' : undefined),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think either. I would add a small comment here in the code to outline why we are checking for ((['dev', 'dev:exec', 'dev:trace'] to help our future selfs when touching this code again πŸ˜‚

@erezrokah erezrokah added the automerge Add to Kodiak auto merge queue label Jan 17, 2022
Copy link
Collaborator

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

Thx for adding the comment! πŸŽ‰

@kodiakhq kodiakhq bot merged commit 898cd5a into main Jan 18, 2022
@kodiakhq kodiakhq bot deleted the fix/build_context branch January 18, 2022 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Add to Kodiak auto merge queue type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug(command-build): unable to set build context via the CONTEXT env flag
2 participants