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

add inheritEnv action parameter #295

Merged
merged 5 commits into from
Jul 18, 2024

Conversation

OmarTawfik
Copy link
Contributor

@OmarTawfik OmarTawfik commented Jul 8, 2024

Upon some further discussion and great insights from @jkeech, I think an opt-in experience (i.e. autoEnv or inerhitEnv) could make good sense. This way it doesn't change the default experience / dev container expectations for all scenarios, but it'd be a helpful option in ones such as those detailed above. And if env was used, it could be treated as an override to anything that was automatically inherited.

Originally posted by @bamurtaugh in #242 (comment)

@OmarTawfik OmarTawfik marked this pull request as ready for review July 8, 2024 07:04
@OmarTawfik OmarTawfik requested review from stuartleeks and a team as code owners July 8, 2024 07:04
@OmarTawfik
Copy link
Contributor Author

cc @bamurtaugh and @stuartleeks

Copy link
Collaborator

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Great, thanks!

Could you also add it to:

  • the table in azdo-task/README.md
  • azdo-task/DevcontainersCi/task.json
  • the table in docs/azure-devops-task.md

@OmarTawfik OmarTawfik requested a review from chrmarti July 9, 2024 15:43
@OmarTawfik
Copy link
Contributor Author

@chrmarti done. Thanks.

common/src/envvars.ts Outdated Show resolved Hide resolved
common/src/envvars.ts Show resolved Hide resolved
@OmarTawfik OmarTawfik requested a review from chrmarti July 13, 2024 08:26
@OmarTawfik
Copy link
Contributor Author

@chrmarti done. Thanks.

chrmarti
chrmarti previously approved these changes Jul 17, 2024
Copy link
Collaborator

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@chrmarti
Copy link
Collaborator

CI seems stuck, closing and reopening to trigger a new run.

@chrmarti chrmarti closed this Jul 17, 2024
@chrmarti chrmarti reopened this Jul 17, 2024
@chrmarti
Copy link
Collaborator

CI failed with:

      DataCloneError: [object Object] could not be cloned.
  
        53 |
        54 | 	test('inherits process env when asked', () => {
      > 55 | 		const originalEnv = structuredClone(process.env);
           | 		                    ^
        56 | 		try {
        57 | 			process.env = {TEST_ENV1: 'value1'};
        58 | 			const input = ['TEST_ENV2=value2'];
  
        at Object.<anonymous> (__tests__/envvars.test.ts:55:23)

@OmarTawfik
Copy link
Contributor Author

OmarTawfik commented Jul 17, 2024

@chrmarti fixed test by removing structuredClone(). The old object is preserved.
Thanks!

@OmarTawfik OmarTawfik requested a review from chrmarti July 17, 2024 12:09
@chrmarti chrmarti closed this Jul 18, 2024
@chrmarti chrmarti reopened this Jul 18, 2024
@chrmarti chrmarti merged commit 4dc5cb4 into devcontainers:main Jul 18, 2024
25 checks passed
@OmarTawfik
Copy link
Contributor Author

OmarTawfik commented Jul 30, 2024

@chrmarti can you please tell me when does this get released?
The last release was 3 months ago.
Thanks!

@OmarTawfik
Copy link
Contributor Author

Is it possible to ship a new release for this? unfortunately this repo cannot be imported/used from source. Thanks!
cc @bamurtaugh or @DonJayamanne

@Sam13
Copy link

Sam13 commented Aug 15, 2024

Is it possible to ship a new release for this? unfortunately this repo cannot be imported/used from source. Thanks! cc @bamurtaugh or @DonJayamanne

+1

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