pixelzoom
commented
3 years ago Discussed at 4/22/21 phet-io meeting. @kathy-phet would @zepumph to identify a fix, then evaluate whether to include in the 1.3 release.
Closed pixelzoom closed 3 years ago
pixelzoom
commented
3 years ago Discussed at 4/22/21 phet-io meeting. @kathy-phet would @zepumph to identify a fix, then evaluate whether to include in the 1.3 release.
zepumph
commented
3 years ago Looks like a potential CORS problem trying to access phet-io.colorado.edu from phet-dev, I will take a look after meetings.
zepumph
commented
3 years ago @mattpen helped me discover that this was in fact a 401 authentication problem under the thinly veiled guise of a CORS issue. When our phet-io server doesn't authenticate, it also doesn't include the needed CORS header that it normally does.
For example, when MP was pinged the phet-io json api with credentials:
curl -I -u $CREDS https://phet-io.colorado.edu/sims/natural-selection/1.2/natural-selection-phet-io-api.json
HTTP/1.1 200 OK
Date: Thu, 22 Apr 2021 20:54:09 GMT
Server: Apache
Last-Modified: Tue, 29 Sep 2020 16:38:01 GMT
ETag: "154f73-5b07668065b92"
Accept-Ranges: bytes
Content-Length: 1396595
Access-Control-Allow-Origin: * ///// see this here!!!
Connection: close
Content-Type: application/json
But when we did so without credentials:
$ curl -I https://phet-io.colorado.edu/sims/natural-selection/1.2/natural-selection-phet-io-api.json
HTTP/1.1 401 Unauthorized
Date: Thu, 22 Apr 2021 20:53:30 GMT
Server: Apache
WWW-Authenticate: Basic realm="PhET-iO Password Protected Area"
Connection: close
Content-Type: text/html; charset=iso-8859-1no CORS header because we need to authenticate. Looks like the Chrome api cares about CORS before it cares about authentication, so that is why we would get this error, when really the request was a 401 unauthorized.
Access to XMLHttpRequest at 'https://phet-io.colorado.edu/sims/natural-selection/1.2.0/natural-selection-phet-io-api.json' from origin 'http://localhost:8080' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.
Looks like the last time we messed with password protection was early 2020, so likely most recent sims are password protected. We should build into the diff wrapper a way to authenticate the requested resource. I'll make a new issue in phet-io-wrappers to figure this out.
UPDATE: also tagging https://github.com/phetsims/phet-io-website/issues/115 since it is the most recent I have thought about password protection in phet-io deploys.
pixelzoom
commented
3 years ago It sounds like there's also some room for improvement of error handling in the Diff wrapper. A problem like that should definitely not result in something that looks like success and "no API changes".
zepumph
commented
3 years ago Agreed!
pixelzoom
commented
3 years ago This issue is dependent on resolution of https://github.com/phetsims/phet-io-wrappers/issues/405.
In https://github.com/phetsims/phet-io-wrappers/issues/405#issuecomment-826008418, @zepumph described that issue as blocking:
To fix that, we need this issue to be working though, so both bugs are important improvements, and both blocking NS release.
In 4/23/21 Slack, @zepumph described that issue as NOT blocking:
- https://github.com/phetsims/phet-io-wrappers/issues/405 is pretty much non-blocking for the natural selection at this point, so long as the UX is acceptable for this NS branch.
@zepumph @kathy-phet please clarify: Is this blocking for NS 1.3 ?
zepumph
commented
3 years ago Yes, the central point of this issue, in which the diff wrapper didn't identify changes, comes from https://github.com/phetsims/phet-io-wrappers/issues/406, not authentication issues in https://github.com/phetsims/phet-io-wrappers/issues/405. So however you want to set up NS specific issues is fine (I see https://github.com/phetsims/natural-selection/issues/274), but https://github.com/phetsims/phet-io-wrappers/issues/406 certainly blocks NS.
pixelzoom
commented
3 years ago Over in https://github.com/phetsims/phet-io-wrappers/issues/405#issuecomment-827037164, @zepumph said regarding phetsims/phet-io-wrappers#405:
... This step unblocked NS for this issue. The diff wrapper now accepts username/password so that when you reference a version from a different domain, you and input the credentials for it. This unblocks NS in my opinion, for this issue.
So I'm going assume that this issue can be closed, and that #274 is sufficient for tracking the "broken diff wrapper" problem. @zepumph If that's incorrect, please reopening this issue.
Closing.
pixelzoom
commented
3 years ago Oh wait... Since this was the original report by QA, I think it's probably better to keep this issue open, and close #274.
To summarize:
pixelzoom
commented
3 years ago From https://github.com/phetsims/phet-io-wrappers/issues/406, a fix is ready. The commits to cherry-pick are:
https://github.com/phetsims/chipper/commit/22fede37cc4e3ec48059bf1756200a80b307fb01 https://github.com/phetsims/phet-io/commit/2dfc6f54f83c6d40f414a186a261c1b23f4b8dd1 https://github.com/phetsims/phet-io-wrappers/commit/09add96755594cfa474ba5ce3cad4a50df336f8f
This will require a new chipper branch named "natural-selection-1.3".
pixelzoom
commented
3 years ago Merge confliics when attemoting to cherry-pick https://github.com/phetsims/chipper/commit/22fede37cc4e3ec48059bf1756200a80b307fb01:
% cd chipper
% git checkout -b natural-selection-1.3
Switched to a new branch 'natural-selection-1.3'
% git cherry-pick 22fede37cc4e3ec48059bf1756200a80b307fb01
error: could not apply 22fede37... Use an IIFE to avoid global variable collisions when the script is loaded more than once, see https://github.com/phetsims/phet-io-wrappers/issues/406
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'In Slack, @zepumph said:
Have to cheery pick for 405 too. Right!
... which presumably means https://github.com/phetsims/phet-io-wrappers/issues/406 and:
https://github.com/phetsims/phet-io-wrappers/commit/2c2bc9ec6d5f423bf4b70e28e810557f49d31e9d
But that does not explain the conflict in chipper.
On hold until someone from the PhET-iO team can work with me on Zoom. I've requested assistenance on Slack phet-io channel.
pixelzoom
commented
3 years ago Zoom meeting: @samreid @zepumph @kathy-phet @pixelzoom
@kathy-phet would like to deliver a working Diff wrapper with NS 1.3
@zepumph advised against trying to move forward to the new PhET-iO API format. It's cosmetic and hasn't been sufficiently vetted by QA.
@zepumph and @samreid will handle patching NS 1.3 to meet the Diff Wrapper requirements.
I'm going to consolidate the various natural-selection GitHub issues related to the Diff Wrapper into this one issue.
The general issues that need to be addressed are:
zepumph
commented
3 years ago Minor update on the above. @samreid and I completed the diff wrapper requirements on master in https://github.com/phetsims/phet-io-wrappers/issues/410. This issue, that we made before we saw phetsims/phet-io-wrappers#408 or phetsims/phet-io-wrappers#409, encompasses both of those issues.
Thus, the issues that will require cherry picking are:
https://github.com/phetsims/phet-io-wrappers/issues/405 https://github.com/phetsims/phet-io-wrappers/issues/406 https://github.com/phetsims/phet-io-wrappers/issues/407 https://github.com/phetsims/phet-io-wrappers/issues/410
UPDATE from @samreid: let's also include https://github.com/phetsims/phet-io-wrappers/issues/411
samreid
commented
3 years ago I cherry-picked the commits for the issues mentioned in the preceding comment. Here are my notes for which commits were applied and which were skipped:
405 https://github.com/phetsims/phet-io-wrappers/commit/2c2bc9ec6d5f423bf4b70e28e810557f49d31e9d (applied)
406 https://github.com/phetsims/chipper/commit/22fede37cc4e3ec48059bf1756200a80b307fb01 (skip) https://github.com/phetsims/phet-io/commit/2dfc6f54f83c6d40f414a186a261c1b23f4b8dd1 (skip) https://github.com/phetsims/phet-io-wrappers/commit/09add96755594cfa474ba5ce3cad4a50df336f8f (applied)
407 https://github.com/phetsims/phet-io-wrappers/commit/94dcc1aac0381127fa3028aac59a7842f6c5df2e (applied)
410 https://github.com/phetsims/phet-io-wrappers/commit/ed674a3de78f45cec323b0e6b994f64d0d52c077 (applied, this had merge conflicts and required manual resolution) https://github.com/phetsims/chipper/commit/338f4d2358c16eb79b6c2b8bcaae50f30b861bd7 (skip, seems unnecessary for the branch)
411 https://github.com/phetsims/chipper/commit/51a091ac8d45d16ca2c60db9c1657a2e99cf3b5e (just in the branch, not cherry-picked)
In summary, there were 4 cherry-picks for phet-io-wrappers, one of which required manual merge resolution. And there was one independent commit for chipper (not a cherry-pick).
Here's what I've done to test these changes: Run local unbuilt and compare to
Run local built and compare to the same list.
Every test passed, but in one run a whitespace in the URL led to difficulty--I think that can be fixed in master and left alone in the branch. I'll make a new issue for that.
I think the next step for this issue is for @zepumph to review these changes and test, then we can move to RC.
pixelzoom
commented
3 years ago @zepumph @samreid When this issue is ready for QA, please provide the follow. I will use the info you provide here to verify the RC before passing it on to QA.
Enumerate what needs to be verified (cosmetic changes, diff output, etc.) and specific steps to follow for verification. Include screenshots were applicable.
Run the Diff wrapper for 1.2 vs 1.3 (a local build), verify that the output is correct, and put the output in this issue. That output will be compared to the RC, to confirm that output is identical.
pixelzoom
commented
3 years ago @zepumph @samreid note that as soon as the Diff wrapper is fixed, and before the RC is published, I'll be using the Diff wrapper to identify changes, so that we can proceed with reviewing those changes for https://github.com/phetsims/natural-selection/issues/275.
zepumph
commented
3 years ago @samreid and I reviewed his changes from last night, and retested all links (on the build and unbuilt sim):
Run local unbuilt and compare to. . .
grunt generate-phet-io-api to see that diff)Run local built and compare to. . .
Everything is looking really good on our end.
zepumph
commented
3 years ago Here are our instructions for how QA should test this:
When comparing to 1.2 and 1.3, you will need to specify the Username and Password (same as if you were accessing the phet-io wrapper index). Use https://phet-io.colorado.edu/sims/natural-selection/1.2 as the link to 1.2.
When comparing to 1.2 with "All Changes". You should see changes that 1.3 adds in green (e.g. gotPointerCaptureAction, lostPointerCaptureAction, phetioDesigned: false). Things that were deleted from 1.2 are shown in red. Like this picture:
,
Verify that the following values for the "Old Sim URL" work as expected, in accordance with the above notes.
[ ] https://phet-io.colorado.edu/sims/natural-selection/1.2/
[ ] https://phet-dev.colorado.edu/html/natural-selection/1.3.0-rc.1/phet-io/
[ ] https://phet-dev.colorado.edu/html/natural-selection/1.3.0-rc.2/phet-io
[ ] https://phet-io.colorado.edu/sims/natural-selection/1.2/natural-selection-phet-io-api.json
pixelzoom
commented
3 years ago I built a local version of NS 1.3:
% cd perennial
% grunt checkout-target --repo=natural-selection --target=1.3
% cd ../natural-selection
% grunt --brands=phet-io... and verified using the instructions in https://github.com/phetsims/natural-selection/issues/271#issuecomment-828622111. This is ready for QA in the next RC test.
pixelzoom
commented
3 years ago Deploy of the Diff wrapper is still broken, see https://github.com/phetsims/phet-io-wrappers/issues/405.
This is no longer ready for QA. @KatieWoe please skip this issue for phetsims/QA#643 until further notice.
pixelzoom
commented
3 years ago @KatieWoe Instructions for QA verification are in https://github.com/phetsims/natural-selection/issues/271#issuecomment-828622111. Let me or @zepumph know if you have questions.
KatieWoe
commented
3 years ago Looks good in rc.3
For https://github.com/phetsims/QA/issues/641#issuecomment-824327481
In https://github.com/phetsims/QA/issues/641#issuecomment-824327481, @KatieWoe asked:
No, that is definitely not correct.
Here's what was specified in https://github.com/phetsims/QA/issues/641:
There have most definitely been API changes since 1.2. For example, in https://github.com/phetsims/natural-selection/issues/263 the Studio tree structure was changed by introducing
environmentPanelas a new "container" node. And those changes have resulted in QA creating issues like https://github.com/phetsims/natural-selection/issues/268.@zepumph can you comment on why the diff wrapper is showing no changes between 1.2 and 1.3.0-rc.1 ?