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

fix(apm/publish): check if artifact.json already exist #175

Merged
merged 2 commits into from Sep 14, 2018

Conversation

Projects
None yet
3 participants
@PascalPrecht
Copy link
Contributor

PascalPrecht commented Aug 16, 2018

@sohkai just a first stab on solving #159 and figuring out whether this is the direction you were aiming for. I'm not super familiar with the publishing process yet and still have to see what's the best way to test this locally, but hopefully this gives us a base for discussing the desired behavior.

See my inline comments.

Closes #159

return `Saved artifact in ${dir}/artifact.json`
if (pathExistsSync(`${ctx.pathToPublish}/${MANIFEST_FILE}`) &&
pathExistsSync(`${ctx.pathToPublish}/${ARTIFACT_FILE}`)) {
task.skip('Reusing existing artifact');

This comment has been minimized.

@PascalPrecht

PascalPrecht Aug 16, 2018

Contributor

I first thought we might wanna take advantage of the dedicated skip() function here, but it seems that API doesn't give us access to ctx which we need to look at the correct path. That's my I went for task.skip() within the task() handler.

Also notice that there's no warning for the user with an option yet to decide whether an existing artifact should be reused or not. I assume this can be easily added with modules like prompt-choice. I first wanted to verify and double check with you if this is the right places for those changes.

Sidenote: I'm unaware of what onlyContent in the context of this command means, if you have some insights on that, that'd be great for me for better understanding :)

This comment has been minimized.

@PascalPrecht

PascalPrecht Aug 16, 2018

Contributor

but it seems that API doesn't give us access to ctx which we need to look at the correct path.

Ha. According to a different spot in the API docs, ctx seems to be available in skip(), so this part can potentially be moved out of the handler entirely (which I think would be nicer anyways) https://github.com/SamVerschueren/listr#task-object

if (pathExistsSync(`${ctx.pathToPublish}/${MANIFEST_FILE}`) &&
pathExistsSync(`${ctx.pathToPublish}/${ARTIFACT_FILE}`)) {
task.skip('Reusing existing artifact');
} else {

This comment has been minimized.

@PascalPrecht

PascalPrecht Aug 16, 2018

Contributor

We might not need this else block in case task.skip() prevents further execution

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Aug 21, 2018

@PascalPrecht Thanks for working on this! I think for the functionality currently in this PR (skipping artifact generation if one already exists), it'd be a great use of a user prompt.

However, #159 is actually about getting aragon apm publish --only-artifact to detect that the current directory doesn't have a artifact.json when someone tries to deploy, and show the user a warning prompt (to either abort or generate it).

This was an issue when I tried to deploy some of our apps, without realizing I was missing the artifact.json (see section for vault.aragonpm.eth in https://github.com/aragon/deployments/blob/master/rinkeby/README.md#aragonpmeth-repo-updates-1)

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 3, 2018

@sohkai thanks for getting back to me here. I'm back from vacation now and will look into this.

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 3, 2018

However, #159 is actually about getting aragon apm publish --only-artifact to detect that the current directory doesn't have a artifact.json when someone tries to deploy, and show the user a warning prompt (to either abort or generate it).

@sohkai Oh I get it now, this was not clear to me when reading the issue - sorry! I made a totally different thing haha.

Also, I've noticed that, when --only-artifacts is given, the actual publish task is not executed anyways: https://github.com/aragon/aragon-cli/blob/master/src/commands/apm_cmds/publish.js#L404

I was a bit confused when I tried to reproduce this. I created a new aragon app using aragon init which doesn't come with an artifact.json by default. The artifact.json is then generated into a temporary directory at the time of publishing, which is then going to be removed once the process finishes, resulting in a state where still no artifact exists in the app folder.

So if I'm getting this right, a user would want to run aragon apm publish --only-artifacts, to generate that non-existing artifact (to make it persistent in the app folder) because it doesn't exist, right?

Isn't a warning that tells the user that no artifact exists a little bit unnecessary then? The command was executed due to non-existence in the first place no?

I'm probably getting something wrong.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Sep 4, 2018

@PascalPrecht Sorry, I definitely got things mixed up in that last reply 😅.

I think what I meant to say was the --only-content flag, like in this publish command:

apm publish 0x0000000000000000000000000000000000000000 --network rinkeby --apm.ens-registry "0xfbae32d1cde62858bc45f51efc8cc4fa1415447e" --no-ipfs-check --apm.ipfs.rpc "http://ipfs.aragon.network:5001" --only-content --no-build

In the linked deployment notes for the Vault, I had to first build the artifact.json via --only-artifacts and then publish via --only-content.

When I initially published 1.0.4 with --only-content, I hadn't generated the artifact.json yet, and so the deployment was incomplete.

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 5, 2018

@sohkai and all of a sudden, things make sense! Will update the PR accordingly.

Unfortunately, Listr eats anything that's being send to stdout within tasks, making it kinda impossible to implement a prompt that asks the user for action. Will have to dig deeper on this one to figure out why that is and if and how we can work around that...

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 5, 2018

Okay, so Listr supports renderers that control how output is being rendered. It also comes with a couple of pre-implemented ones, one of them being the verbose-renderer.

As you might've guessed, it pretty much renders everything that's being emitted by tasks and it also works correctly with an integrated prompt.

The downside however is that it really prints out everything, so I think, if we'd want the nice looking task list + a nice prompt, but no verbose output, we'll have to build our own renderer. Unfortunately, there's no way to distinguish between "normal" stdout data and a prompt, so I'm not sure if that can be solved with the available APIs without going a down a super hacky road.

There's two other options I could think of to get this feature in and move on (for now):

  1. We don't do a prompt at all and simply abort with an error message if no artifact exists, also telling the user to generate one first (and how).
  2. We could perform the check before the tasklist is even initialized, have a prompt and either exit early (before the task list is initialized), or always generate the artifact respectively.

Hope this makes sense.

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 6, 2018

Just realized that option 2) requires dirToPublish to exist, which in the case of using --only-content is a temporary folder generated as part of the task. We might not want to do this work up front, so I guess option 1) would be the most pragmatic to move forward with this one for now.

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:fix/commands/apm/publish branch from 07b3f49 to 5ec911c Sep 6, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 6, 2018

Okay, I now went ahead and implemented 2), simply throwing with a meaningful error message in case artifact.json doesn't exist.

I've also removed the first implementations to let the user know that an existing artifact is being reused. The reason being is that

  1. I misunderstood the original issue (basically I was solving a different problem that isn't really a problem)
  2. Even if we'd keep it, we'd want a prompt there as well to ask the user whether the artifact should be re-generated or not. This is something that doesn't seem to be feasible to implement right now, as mentioned in #175 (comment)

If you do prefer getting that stuff in, let me know and we can revisit.

@PascalPrecht PascalPrecht changed the title WIP(apm/publish): check if artifact.json or manifest.json already exist fix(apm/publish): check if artifact.json already exist Sep 6, 2018

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:fix/commands/apm/publish branch from b43e73d to 59f1113 Sep 10, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 10, 2018

I did a bit more research and it turned out that there is already a package that adds rudimentary prompt input support to listr. I've added another commit that adds it to the publish command. Here's what it looks like:

sep-10-2018 11-20-46

I think this should do it. Let me know if this needs more change (probably changes in wording are required, and I'll update the PR.

Also, I'll remove the first commit in case we decide going for the version with the prompt.


return `Saved artifact in ${dir}/artifact.json`
if (onlyContent && !pathExistsSync(`${dir}/${ARTIFACT_FILE}`)) {

This comment has been minimized.

@izqui

izqui Sep 11, 2018

Member

In a publishing without --only-content, which means the contract is being updated, we should always generate a new artifact even if it exists already.

With the current changes, it would never generate artifacts unless the --only-content flag is provided.

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 11, 2018

Contributor

👍

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:fix/commands/apm/publish branch from 59f1113 to 0076f2c Sep 11, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 11, 2018

@izqui updated the PR as requested.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Sep 11, 2018

@PascalPrecht Looks really good! Thanks for doing all that research, and I think it's paved the way for getting a few more annoying things with the CLI out of the way :).

As for

I misunderstood the original issue (basically I was solving a different problem that isn't really a problem)

I'm not sure if this is useful, as AFAIK the artifacts are generally meant to be overridden, but it might be nice as a user option. It should be done separately in another PR though :).

PascalPrecht added some commits Aug 16, 2018

fix(apm/publish): check whether artifact.json already exist
And throw if it doesn't. CLI users will have to generate the artifact.json
explicitly using `apm publish --only-artifacts`.

Closes #159

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:fix/commands/apm/publish branch from 0076f2c to 3cdffb6 Sep 14, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 14, 2018

It should be done separately in another PR though :).

Agreed.

I've rebased this one on latest master. This should be ready for merge unless there's any more requests for change.

@izqui

izqui approved these changes Sep 14, 2018

Copy link
Member

izqui left a comment

LGTM! I'm thinking that the need to provide input could become an annoyance if used inside scripts or a CI. We can work on it later though

@izqui izqui merged commit 75c3fdf into aragon:master Sep 14, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
license/cla Contributor License Agreement is signed.
Details

galactusss added a commit to galactusss/aragon-cli that referenced this pull request Jan 5, 2019

fix(apm/publish): check if artifact.json already exist (aragon#175)
* fix(apm/publish): check whether artifact.json already exist

And throw if it doesn't. CLI users will have to generate the artifact.json
explicitly using `apm publish --only-artifacts`.

Closes aragon#159

* feat(apm/publish): allow users to generate artifact if it doesn't exist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment