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

Fixing package bug breaking liveUpdate tasks over https #728

Merged
merged 3 commits into from
Mar 31, 2022
Merged

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Mar 31, 2022

Summary

Just changing wws => wss as it should have been. Unsure how I missed this in the test plan. Includes updates to prepare Mephisto for a 1.0.1 release and tag.

Implementation

Mostly updating our packages to now refer to 2.0.1 and 1.0.1, other than a single character change.

Testing

Ran the task in sandbox that was having issue before, it no longer does with 2.0.1

Thanks @jxmsML for surfacing the issue.

@JackUrb JackUrb requested a review from pringshia March 31, 2022 21:40
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 31, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2022

Codecov Report

Merging #728 (df7927d) into main (9448ef2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #728      +/-   ##
==========================================
+ Coverage   64.27%   64.28%   +0.01%     
==========================================
  Files         107      107              
  Lines        9200     9200              
==========================================
+ Hits         5913     5914       +1     
+ Misses       3287     3286       -1     
Impacted Files Coverage Δ
...sto/abstractions/architects/router/build_router.py 86.79% <100.00%> (ø)
...tractions/architects/channels/websocket_channel.py 76.56% <0.00%> (-3.13%) ⬇️
mephisto/data_model/agent.py 68.30% <0.00%> (+0.81%) ⬆️
...ephisto/abstractions/_subcomponents/task_runner.py 79.70% <0.00%> (+0.99%) ⬆️

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 9448ef2...df7927d. Read the comment docs.

@JackUrb
Copy link
Contributor Author

JackUrb commented Mar 31, 2022

@pringshia any idea why the mephisto-task-latest check is failing on this pr? I know pre-commit black is failing due to a circle error only recently resolved, but don't see what's wrong otherwise here. Will still merge as the referenced files in this PR have the new 2.0.1 version in the package.json files.

@JackUrb JackUrb merged commit f698523 into main Mar 31, 2022
@JackUrb JackUrb deleted the wws-fix branch March 31, 2022 23:17
@pringshia
Copy link
Contributor

pringshia commented Mar 31, 2022

@JackUrb I'm not sure why the commit at 9448ef28949b63e83a309c9d5cfbb6e4f11b2968 is getting pulled down during that workflow.

And that commit has the older version of the package defined:

Update: It seems the pull_request_target trigger in the GH Action only compares main against main (to avoid security issues), some more info here.

I'll investigate a fix tomorrow.

@pringshia
Copy link
Contributor

@JackUrb - Only the remote procedure and static_react_task's packages were updated to 1.0.1 in this PR. What about parlai chat?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants