[Movey] Add command to upload metadata to Movey #227

ea-open-source · 2022-06-27T09:04:57Z

Motivation

Add move movey-upload command to upload move package to Movey, modify move build command to call Movey API to increase download count.

How to use it

Before you use the new command to upload metadata to Movey, please make sure:

your package must be on Github
the remote must be named “origin”
file $HOME/.move/movey_credential.toml must exist, and the content should be like(you can get the token on Movey):

[registry]
token = "example-token"

Note: you could use the move login command from this PR to save your Movey's api token using the CLI.

Step 1: Navigate to the folder containing your package.
Step 2: Run the following command:
move movey-upload

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

cargo test --package move-cli --test cli_tests upload_package
cargo test --package move-cli --lib get_api_token

ea-open-source · 2022-07-07T12:55:35Z

wrwg, awelc, damirka
Would you guys help us with rerunning Actions and review? We have already contacted Sam and Todd.

sblackshear

Excited to see this, thanks for adding the CLI integration! Looks good to me overall, but requesting changes for some minor hygiene issues to improve maintainability.

language/tools/move-cli/src/base/upload.rs

language/tools/move-cli/src/lib.rs

language/tools/move-cli/src/utils/credential.rs

language/tools/move-cli/src/base/movey_upload.rs

ea-open-source · 2022-07-15T02:43:39Z

@sblackshear All issues were resolved, please check again.

sblackshear

Thanks for your patience! Looking better, requesting changes for:

Removing logic to bump Movey download counter (belongs in a separate PR)
Refactoring for cleaner separation of test upload logic from "real" upload logic

language/tools/move-cli/src/base/movey_upload.rs

language/tools/move-cli/src/utils/credential.rs

language/tools/move-cli/src/base/movey_upload.rs

language/tools/move-cli/src/utils/credential.rs

language/tools/move-package/src/resolution/resolution_graph.rs

ea-open-source · 2022-08-02T11:18:34Z

Hi Sam, we have updated all 3 of our PRs(#226, #227, #318). Please give them another round of review.

awelc · 2022-08-03T23:12:50Z

Hi Sam, we have updated all 3 of our PRs(#226, #227, #318). Please give them another round of review.

I reassigned these to myself and will re-review shortly.

awelc · 2022-08-05T01:08:39Z

language/tools/move-cli/src/utils/movey_credential.rs

+use std::fs;
+use toml_edit::easy::Value;
+
+pub const MOVEY_CREDENTIAL_PATH: &str = "/movey_credential.toml";


This PR partially overlaps with #226

For example the MOVEY_CREDENTIAL_PATH is already defined there in a different file and manipulating of movey_credential.toml is also at least partially duplicated which makes it difficult to review these side-by-side.

My suggestion is that you either make this PR into a stacked diff on top of #226 to refactor the code shared between the two or we simply land #226 first and you redo this one on top of HEAD. Please let me know what you'd prefer to do (and on my side I will try to act as quickly as I can on the subsequent changes).

Hi awelc, good to see you.
Our goal is to get the service going so please merge #226 first and we will refactor #227 and #318 accordingly.

Hi awelc, good to see you. Our goal is to get the service going so please merge #226 first and we will refactor #227 and #318 accordingly

I reviewed #226 but it still requires some work - that is why I offered an option of creating a stacked diff for your consideration. In particular, I don't think it will run on Windows which is important as a large portion of Move developers seems to work on Windows as indicated by our internal surveys (and we plan to enable Windows CI builds soon for the core Move repo).

In that case, we will look into #226 and make it Windows compatible, particularly with the location of the credential file.
In the meantime, please merge #227 as that's the more important PR.

In that case, we will look into #226 and make it Windows compatible, particularly with the location of the credential file. In the meantime, please merge #227 as that's the more important PR.

Both PRs suffer from similar problems (e.g., in terms of concatenating paths that in my opinion will not work on Windows), so neither can be merged right away. I did a review of #277, and we can reverse the order and try to land #277 first (and merge #266 on top of it later, trying to remove existing code duplication), but I will not be able to work on these any more tonight.

Also, I tried upload and tried to upload a package (https://github.com/MystenLabs/sui/tree/main/sui_programmability/examples/move_tutorial) and apparently it was uploaded successfully but I can't find it in Movey...

~/east-login-move/target/debug/move movey-upload Your package has been successfully uploaded to Movey

Thanks awelc, we will work on these and have them ready within today. It's Friday for us so we won't be able to response to you tomorrow.
About your uploaded package we believed you uploaded to staging instead of production.
It's here https://movey-app-staging.herokuapp.com/packages/MyFirstPackage
since we read from the move.toml file the package name won't be the same as your folder name.

Thanks awelc, we will work on these and have them ready within today. It's Friday for us so we won't be able to response to you tomorrow.

Please make sure that it runs on Windows (I will try to re-test, but it would be good if you did it yourselves).

About your uploaded package we believed you uploaded to staging instead of production. It's here https://movey-app-staging.herokuapp.com/packages/MyFirstPackage since we read from the move.toml file the package name won't be the same as your folder name.

Does it mean that the data hard-coded in the source points to staging and will have to be changed in the future? I was a little worried about all this data being hard-coded and now I am worried even more...

The movey credentials are here https://github.com/move-language/move/blob/62f645c4381848f42c4b03fc3a6d510d8a0263ff/language/move-command-line-common/src/movey_constants.rs
Since you are building under debug mode it defaulted to the staging endpoint. Under prod it will point to Movey.net so I don't think you need to be worried over hardcoded values.

The movey credentials are here https://github.com/move-language/move/blob/62f645c4381848f42c4b03fc3a6d510d8a0263ff/language/move-command-line-common/src/movey_constants.rs Since you are building under debug mode it defaulted to the staging endpoint. Under prod it will point to Movey.net so I don't think you need to be worried over hardcoded values.

Cool, makes perfect sense! Thank you for the explanation!

According to our tests, everything is working and up to date. Both PRs are designed to work independently. We will rebase once either is merged.

language/tools/move-cli/src/base/movey_upload.rs

language/tools/move-cli/src/utils/movey_credential.rs

language/tools/move-cli/tests/cli_tests.rs

awelc · 2022-08-05T04:32:05Z

language/tools/move-cli/src/utils/movey_credential.rs

+use std::fs;
+use toml_edit::easy::Value;
+
+pub const MOVEY_CREDENTIAL_PATH: &str = "/movey_credential.toml";


In that case, we will look into #226 and make it Windows compatible, particularly with the location of the credential file. In the meantime, please merge #227 as that's the more important PR.

Both PRs suffer from similar problems (e.g., in terms of concatenating paths that in my opinion will not work on Windows), so neither can be merged right away. I did a review of #277, and we can reverse the order and try to land #277 first (and merge #266 on top of it later, trying to remove existing code duplication), but I will not be able to work on these any more tonight.

Also, I tried upload and tried to upload a package (https://github.com/MystenLabs/sui/tree/main/sui_programmability/examples/move_tutorial) and apparently it was uploaded successfully but I can't find it in Movey...

~/east-login-move/target/debug/move movey-upload Your package has been successfully uploaded to Movey

awelc · 2022-08-08T22:21:52Z

I have merged #226 and, as a result, this branch now has conflicts. While resolving these, please refactor Movey configuration file access routines so that the functionality to create/read the file (for both the login command and for the upload command are in the same place).

# Conflicts: # Cargo.lock # language/tools/move-cli/Cargo.toml

# Conflicts: # Cargo.lock # language/tools/move-cli/tests/cli_tests.rs

# Conflicts: # Cargo.lock

# Conflicts: # language/tools/move-cli/tests/cli_tests.rs

…y staging url

# Conflicts: # language/tools/move-package/Cargo.toml # language/tools/move-package/src/resolution/resolution_graph.rs

# Conflicts: # language/tools/move-package/src/resolution/resolution_graph.rs

- MoveyUploadRequest: +subdir, -rev - Remove integration test of head commit id

- Add more integration test to Movey upload - Remove unused stub move packages - Upgrade `log` crate version - Add `httpmock` crate

- change `init_server_mock` to `mock_movey_server_with_response_body_and_status_code` to be more inline with the comment - remove `// Copyright (c) The Diem Core Contributors`

awelc

I can still see code duplicated between movey_upload.rs and movey_login.rs and I don't see a good reason for it. Perhaps you could have both CLI commands use code in movey_credentials.rs (after some additional refactoring).

awelc · 2022-08-10T17:50:02Z

language/tools/move-cli/src/utils/movey_credential.rs

+use std::fs;
+use toml_edit::easy::Value;
+
+pub const MOVEY_CREDENTIAL_PATH: &str = "/movey_credential.toml";


This constant is also defined in movey_login.rs.

awelc · 2022-08-10T17:53:30Z

language/tools/move-cli/src/utils/movey_credential.rs

+}
+
+pub fn get_api_token(move_home: &str) -> Result<String> {
+    let credential_path = format!("{}{}", move_home, MOVEY_CREDENTIAL_PATH);


The code reading (or attempting to read the credentials file) is very similar to the one in movey_login.rs.

…ding credential file

awelc

Looks good! Thank you for your patience!

sblackshear requested review from awelc, damirka, tnowacki and wrwg June 27, 2022 16:32

ea-open-source force-pushed the features/upload-command branch 3 times, most recently from 45658b8 to b650483 Compare July 7, 2022 12:53

sblackshear suggested changes Jul 8, 2022

View reviewed changes

ea-open-source force-pushed the features/upload-command branch from 10e9d1d to aeef732 Compare July 13, 2022 16:09

oyster reviewed Jul 14, 2022

View reviewed changes

language/tools/move-cli/src/base/movey_upload.rs Outdated Show resolved Hide resolved

ea-open-source mentioned this pull request Jul 15, 2022

[Movey] Add command to save Movey API Token #226

Merged

sblackshear suggested changes Jul 17, 2022

View reviewed changes

ea-open-source changed the title ~~Add command to upload metadata to Movey~~ [Movey] Add command to upload metadata to Movey Jul 18, 2022

ea-open-source force-pushed the features/upload-command branch from be77239 to 629ef80 Compare July 28, 2022 02:08

ea-open-source mentioned this pull request Jul 29, 2022

[Movey] Call Movey API to increase download count #318

Closed

ea-open-source force-pushed the features/upload-command branch from 570cd26 to 62f645c Compare August 2, 2022 10:57

awelc self-assigned this Aug 3, 2022

awelc suggested changes Aug 5, 2022

View reviewed changes

ea-open-source added 6 commits August 9, 2022 15:32

upload metadata to movey

532531f

# Conflicts: # Cargo.lock # language/tools/move-cli/Cargo.toml

Add api token to upload request, get token from credential file

e1091b2

# Conflicts: # Cargo.lock # language/tools/move-cli/tests/cli_tests.rs

Show error message if upload with bad credentials

1213fbe

# Conflicts: # Cargo.lock

Refactor move_cli's utils & add cli tests

4c4d756

# Conflicts: # language/tools/move-cli/tests/cli_tests.rs

Remove description, total_size field from request body, change move…

fe74142

…y staging url

call movey api after getting dependency

3eab134

# Conflicts: # language/tools/move-package/Cargo.toml # language/tools/move-package/src/resolution/resolution_graph.rs

ea-open-source added 14 commits August 9, 2022 15:34

Change dev movey_url, send params in post request

5b63f9d

# Conflicts: # language/tools/move-package/src/resolution/resolution_graph.rs

Refactor error message when upload failed, add reqwest crate

cd20142

Get long rev, use cli_exe path from env

0d374f6

Refactor test code to run in parallel

c99c9b5

Refactor tests to run in current dir, flatten upload command

38eefef

Refactor to be specific to Movey, change UploadRequest's params

b575537

- MoveyUploadRequest: +subdir, -rev - Remove integration test of head commit id

Refactor MOVE_HOME env, MOVEY_URL to common package

1bdf00d

Refactor test logic in uploading, remove increase download logic

9f1c462

Add Movey server Mock, refactor test code in upload

ccc39c6

- Add more integration test to Movey upload - Remove unused stub move packages - Upgrade `log` crate version - Add `httpmock` crate

Rename move_command_line_common::movey.rs to movey_constants.rs

e98bb73

Change movey api url, refactor getting credential key, refactor tests

9680b91

Rename movey api, change the message of successful upload request

41d4298

Refactor and add comments for movey tests

6fd45f5

Refactor and rebase according to move-language#226 changes

190f3a2

ea-open-source force-pushed the features/upload-command branch from 9ed5de5 to 190f3a2 Compare August 10, 2022 02:35

Refactor mock movey server function, remove unnecessary license

7a63283

- change `init_server_mock` to `mock_movey_server_with_response_body_and_status_code` to be more inline with the comment - remove `// Copyright (c) The Diem Core Contributors`

awelc suggested changes Aug 10, 2022

View reviewed changes

Move MOVEY_CREDENTIAL_PATH to common package, refactor logic of rea…

e94be27

…ding credential file

awelc approved these changes Aug 11, 2022

View reviewed changes

sblackshear approved these changes Aug 12, 2022

View reviewed changes

awelc merged commit 165e1d3 into move-language:main Aug 12, 2022

brson pushed a commit to brson/move that referenced this pull request Jul 12, 2023

Enabling compilation from source (.move) (move-language#227)

cfff11c

brson pushed a commit to brson/move that referenced this pull request Jul 17, 2023

Enabling compilation from source (.move) (move-language#227)

7a794b5

[Movey] Add command to upload metadata to Movey #227

[Movey] Add command to upload metadata to Movey #227

Uh oh!

Conversation

ea-open-source commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

How to use it

Have you read the Contributing Guidelines on pull requests?

Test Plan

Uh oh!

ea-open-source commented Jul 7, 2022

Uh oh!

sblackshear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ea-open-source commented Jul 15, 2022

Uh oh!

sblackshear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ea-open-source commented Aug 2, 2022

Uh oh!

awelc commented Aug 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ea-open-source Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awelc Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awelc commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awelc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awelc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

ea-open-source commented Jun 27, 2022 •

edited

Loading

ea-open-source Aug 5, 2022 •

edited

Loading

awelc Aug 5, 2022 •

edited

Loading

awelc commented Aug 8, 2022 •

edited

Loading