ACNA-882 - Rewriting files during app run is error prone, and leads to unexpected issues #309

shazron · 2020-10-23T10:06:23Z

Fixes #275

How Has This Been Tested?

npm test
manual test on macOS (browser action test, vscode local debugging test)
manual test on Linux Ubuntu 20 (see workaround linux: aio app run --local testing in the browser issue #311) (browser action test, vscode local debugging test)
manual test on Windows 10 (re-fixed Windows: docker not found when running aio app run --local --verbose (openwhisk) #304 via re-fix: add new Windows docker.exe location apache/openwhisk#5021)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

…pected issues 1. during --verbose, java version is not shown since that output is to stderr (and we use stdout). fixed. 2. 100% test coverage

codecov · 2020-10-27T03:57:08Z

Codecov Report

Merging #309 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #309   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        29    +2     
  Lines         1259      1255    -4     
  Branches       219       206   -13     
=========================================
- Hits          1259      1255    -4

Impacted Files	Coverage Δ
src/lib/app-helper.js	`100.00% <100.00%> (ø)`
src/lib/cleanup.js	`100.00% <100.00%> (ø)`
src/lib/config-loader.js	`100.00% <100.00%> (ø)`
src/lib/poller.js	`100.00% <100.00%> (ø)`
src/lib/runDev.js	`100.00% <100.00%> (ø)`
src/lib/vscode.js	`100.00% <100.00%> (ø)`

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 ef6c2a3...f868c95. Read the comment docs.

purplecabbage · 2020-10-28T06:20:07Z

I find this potentially more confusing. Now I don't have a .vscode/launch.json and then I do, and then I don't ...
Why can't launch.json be persistent? It does not change between aio app run and aio app run --local

Also, with .env file now in dist/ ... we have one which is a copy of ../.env when run without --local, and one with ... can we just keep a local.env and prod.env in the dist/ folder instead?

shazron · 2020-10-29T00:18:19Z

I find this potentially more confusing. Now I don't have a .vscode/launch.json and then I do, and then I don't ...
Why can't launch.json be persistent? It does not change between aio app run and aio app run --local

I didn't change the existing functionality, so this behaviour is consistent with the previous behaviour. Not sure the rationale for this functionality.

Also, with .env file now in dist/ ... we have one which is a copy of ../.env when run without --local, and one with ... can we just keep a local.env and prod.env in the dist/ folder instead?

Not sure I follow exactly. When you run with --local it uses dist/.env. When run without --local it uses your root .env and nothing has changed there.

shazron · 2020-11-02T10:43:44Z

Windows 10 manual tests have been done (via a local patch, see Description), and local debugging etc works.

moritzraho

One inline question about the stop log arg.
Didn't tested, just looked at the code for now, but this looks mostly fine to me.

I agree with @purplecabbage about the vscode file, that it should be generated via generator-aio-app (on init), but I think this could be addressed in another PR

About the dist/.env discussion, I guess one potential enhancement would be, if possible, to pass wskdebug vars via environment instead of dist/.env, so that we can rename dist/.env to dist/.env.local (makes the intent clearer for advanced devs looking at dist/) and only have it generated on --local. But again not sure if it's possible or should be done as part of this PR

Also thanks for the extra refactoring and cleanup work !

src/lib/cleanup.js

src/lib/runDev.js

shazron · 2020-11-02T13:33:32Z

I agree with @purplecabbage about the vscode file, that it should be generated via generator-aio-app (on init), but I think this could be addressed in another PR

I left this precisely for another PR, since I saw that comment somewhere about moving to the generator eventually.

About the dist/.env discussion, I guess one potential enhancement would be if possible to pass wskdebug vars via environment instead of dist/.env, so that we can rename dist/.env to dist/.env.local (makes the intent clearer for advanced devs looking at dist/) and only have it generated on --local. But again not sure if it's possible or should be done as part of this PR

We could pass it in as env vars instead of dist/.env, but I'm not sure what the advantage is since dist/.env is functionally equivalent and cleaner. We can rename dist/.env to anything so that is not a problem in renaming it.

moritzraho · 2020-11-02T13:51:25Z

We could pass it in as env vars instead of dist/.env, but I'm not sure what the advantage is since dist/.env is functionally equivalent and cleaner. We can rename dist/.env to anything so that is not a problem in renaming it.

I meant to limit the cases where we generate the extra file, but ok might be more confusing to have multiple means of setting env variables indeed. Maybe we can add some comments in the generated .env instead ?

shazron · 2020-11-03T09:37:16Z

Ok I think I get what you mean Moritz.

for --local generate the dist/.env.local with comments
if --local not present, use the root .env

I will keep the existing .vscode/launch.json functionality since it was the same behaviour before (most likely so we don't potentially clobber their changes).
I will leave the vscode file generation to the generator for another PR.

moritzraho · 2020-11-03T09:40:18Z

ok agree to do this in incremental steps 🙂

…se dist/.env.local (file rename, with comments)

shazron · 2020-11-04T08:54:16Z

Travis-CI builds have stopped on this org. Migrating to Github Actions in another PR, and when that is integrated, we can merge it into this PR.

shazron mentioned this pull request Oct 23, 2020

Rewriting files during app run is error prone, and leads to unexpected issues. #275

Closed

fix: rewriting files during app run is error prone, and leads to unex…

bc7e64a

…pected issues 1. during --verbose, java version is not shown since that output is to stderr (and we use stdout). fixed. 2. 100% test coverage

shazron force-pushed the ACNA-882 branch from bd36b32 to bc7e64a Compare October 27, 2020 03:54

adobe deleted a comment from codecov bot Oct 27, 2020

shazron changed the title ~~WIP - ACNA-882 - Rewriting files during app run is error prone, and leads to unexpected issues~~ ACNA-882 - Rewriting files during app run is error prone, and leads to unexpected issues Oct 27, 2020

Merge branch 'master' into ACNA-882

59c5d2c

shazron marked this pull request as ready for review October 27, 2020 05:39

meryllblanchet requested review from moritzraho and purplecabbage October 27, 2020 08:58

moritzraho reviewed Nov 2, 2020

View reviewed changes

src/lib/cleanup.js Show resolved Hide resolved

src/lib/runDev.js Show resolved Hide resolved

moritzraho self-requested a review November 3, 2020 16:13

moritzraho approved these changes Nov 3, 2020

View reviewed changes

fix: use root .env for remote debugging (not dist/.env). for local, u…

fd350a7

…se dist/.env.local (file rename, with comments)

shazron added 3 commits November 4, 2020 18:11

Merge branch 'master' into ACNA-882

a919314

Merge branch 'master' into ACNA-882

e4a59a2

Merge branch 'master' into ACNA-882

f868c95

shazron merged commit 6c7f62d into master Nov 4, 2020

shazron deleted the ACNA-882 branch November 4, 2020 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ACNA-882 - Rewriting files during app run is error prone, and leads to unexpected issues #309

ACNA-882 - Rewriting files during app run is error prone, and leads to unexpected issues #309

shazron commented Oct 23, 2020 •

edited

Loading

codecov bot commented Oct 27, 2020 •

edited

Loading

purplecabbage commented Oct 28, 2020 •

edited

Loading

shazron commented Oct 29, 2020

shazron commented Nov 2, 2020

moritzraho left a comment •

edited

Loading

shazron commented Nov 2, 2020

moritzraho commented Nov 2, 2020

shazron commented Nov 3, 2020

moritzraho commented Nov 3, 2020

shazron commented Nov 4, 2020

ACNA-882 - Rewriting files during app run is error prone, and leads to unexpected issues #309

ACNA-882 - Rewriting files during app run is error prone, and leads to unexpected issues #309

Conversation

shazron commented Oct 23, 2020 • edited Loading

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Oct 27, 2020 • edited Loading

Codecov Report

purplecabbage commented Oct 28, 2020 • edited Loading

shazron commented Oct 29, 2020

shazron commented Nov 2, 2020

moritzraho left a comment • edited Loading

Choose a reason for hiding this comment

shazron commented Nov 2, 2020

moritzraho commented Nov 2, 2020

shazron commented Nov 3, 2020

moritzraho commented Nov 3, 2020

shazron commented Nov 4, 2020

shazron commented Oct 23, 2020 •

edited

Loading

codecov bot commented Oct 27, 2020 •

edited

Loading

purplecabbage commented Oct 28, 2020 •

edited

Loading

moritzraho left a comment •

edited

Loading