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

Refactor: Add uploadFilesToStorageProvider function to API #37

Closed

Conversation

3 participants
@0xGabi
Copy link
Member

commented Jul 2, 2019

Make possible to output information during aragon apm publish before we actually publish it to AragonPM repo.

@0xGabi 0xGabi added the enhancement label Jul 2, 2019

@0xGabi 0xGabi requested review from sohkai and 0x6431346e Jul 2, 2019

@0xGabi 0xGabi added this to In progress in Aragon Mesh Team via automation Jul 2, 2019

@0x6431346e
Copy link
Member

left a comment

LGTM!

To summarize:

  • this is a breaking change
  • people can migrate by doing uploadFilesToStorageProvider before publishVersion or publishVersionIntent

Right?

I think the CLI is the only component using apm.js to publish, right? 🤔
cc: @sohkai

@0x6431346e 0x6431346e moved this from In progress to Review in Aragon Mesh Team Jul 3, 2019

@0xGabi

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

Yes exactly. The new workflow would be uploadFilesToStorageProvider this will return the contentURI that you need to publishVersion or publishVersionIntent.

I have checked with Brett and he also think that this bit is only used by the aragonCLI.

@@ -11,38 +11,36 @@ const GET_INFO_TIMEOUT = 10000 //ms
module.exports = (web3, options = {}) => {
const defaultOptions = {
ensRegistryAddress: null,
providers: {}
providers: {},

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 4, 2019

Member

For future ease of review, it would be great if the prettier changes were committed separately from the logic changes :).

This comment has been minimized.

Copy link
@0xGabi

0xGabi Jul 4, 2019

Author Member

Sorry about that, I use an extension on vscode to automatically format the code 😅

* @param {string} directory The directory that contains files to publish.
* @return {string} A string representing the contentURI
*/
async uploadFilesToStorageProvider(provider, directory) {

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 4, 2019

Member

I don't really have anything against this, and it is the easiest solution, but it just feels like a bit of an abstraction leak.

An alternative way to achieve this is to add a hook into the publishing methods for an async "confirmation" check, that allows the caller to specify some custom logic once the files have been uploaded but before the publish happens. This would also allow the method to stay backwards compatible (if we really desired it, but no real reason to).

This comment has been minimized.

Copy link
@0xGabi

0xGabi Jul 4, 2019

Author Member

You are right, I been thinking the same about the leak abstraction.

On a second look I believe I'm able to get the contentURI from the intent (here) something like: params[4] 🤔

In that case we don't need this PR at all 🙌

@0xGabi

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

I close this PR cause is no longer needed after: aragon/aragon-cli@93cbb3c#diff-72c73fc10b9d14701bc153f8e841ff0bR654

@0xGabi 0xGabi closed this Jul 5, 2019

@0x6431346e 0x6431346e moved this from Review to Done in Aragon Mesh Team Jul 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.