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.

Sign up for GitHub

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

Jump to bottom

Add support for anchor idl fetch to work outside anchor workspace #1509

Merged
merged 5 commits into from
Mar 12, 2022
Merged

Add support for anchor idl fetch to work outside anchor workspace #1509

merged 5 commits into from
Mar 12, 2022

Conversation

NBNARADHYA
Copy link
Contributor

Context

Closes #1453 (Refer to this issue for context)

Changes

  1. anchor idl fetch can now be run outside an anchor workspace.
  2. If it is run outside workspace, provider.cluster option has to be provided.

@NBNARADHYA
Allow anchor idl fetch to run outside workspace
b643ca8 
Signed-off-by: Nikhil B N <nikhilbn365@gmail.com>
@NBNARADHYA
Copy link
Contributor Author

@paul-schaaf I have made changes. Please suggest any changes if they are required.

Also had a question: How is anchor cli tested ? If I need to write tests for my change, how do I go about this?

Thanks

@paul-schaaf
Copy link
Contributor

paul-schaaf commented Feb 22, 2022 •
edited
Loading

dont think we have to require the provider.cluster to be provided. if it's not given and we're outside a workspace, the cluster from the local solana config can be used

re:cli tests we..just test it manually. But we can start adding tests to the CI. Are you comfortable doing that or do you prefer us to do it? imo it's fine to just merge this without testing and then merge a ci change later but if you want to add tests to tests.yaml now, feel free!

@NBNARADHYA
Copy link
Contributor Author

@paul-schaaf Thanks! On it!

If this works fine, I would prefer for this to be merged! I shall create a new PR with the tests if that's okay!

Thanks

@NBNARADHYA
Copy link
Contributor Author

@paul-schaaf please suggest any changes!

@armaniferrante
Copy link
Member

@armaniferrante

please don't needlessly tag. Someone will review when they have time.

@paul-schaaf
Copy link
Contributor

looks like you found the cli-config solana crate but why did you copy that code into ours instead of importing the crate?

@NBNARADHYA
Copy link
Contributor Author

looks like you found the cli-config solana crate but why did you copy that code into ours instead of importing the crate?

@paul-schaaf makes more sense! I have made changes

paul-schaaf
paul-schaaf reviewed Feb 26, 2022
View reviewed changes
cli/src/config.rs Show resolved Hide resolved
cli/src/lib.rs Outdated Show resolved Hide resolved
cli/src/lib.rs Outdated Show resolved Hide resolved
@paul-schaaf
Copy link
Contributor

pls also add a line to the feature section in the changelog for the unreleased release

@NBNARADHYA
Use default solana config for anchor idl fetch
4b39ef2 
Signed-off-by: Nikhil B N <nikhilbn365@gmail.com>
@NBNARADHYA
Copy link
Contributor Author

@paul-schaaf hope this is done!

@paul-schaaf
Copy link
Contributor

paul-schaaf commented Mar 7, 2022 •
edited
Loading

pls update your changelog and if you do another PR, adjust your PR to allow edits from maintainers so I can just do that and merge instead

@NBNARADHYA
Copy link
Contributor Author

@paul-schaaf I have already added updated the changelog. I am not able to see “allow edits from maintainers” option on this pull request.

@paul-schaaf
Copy link
Contributor

u need to update it again. your change is in the wrong place cause we created a release

@NBNARADHYA
Copy link
Contributor Author

@paul-schaaf done !

@paul-schaaf
Copy link
Contributor

paul-schaaf commented Mar 8, 2022 •
edited
Loading

pls sign your commits and force push again

@NBNARADHYA
Merge branch 'master' of github.com:project-serum/anchor into cli-wor…
c5eec84 
…kspace

Signed-off-by: Nikhil B N <nikhilbn365@gmail.com>
@NBNARADHYA
Copy link
Contributor Author

@paul-schaaf done!

@paul-schaaf
Copy link
Contributor

looks like your commits are still not signed

@NBNARADHYA NBNARADHYA force-pushed the cli-workspace branch 3 times, most recently from 7d99492 to c5eec84 Compare March 8, 2022 20:40
@NBNARADHYA
Copy link
Contributor Author

@paul-schaaf I can see that they are on github

@fanatid
Copy link
Contributor

fanatid commented Mar 9, 2022

@NBNARADHYA they are not, if commits are signed then you will see Verified label
Screenshot from 2022-03-09 11-08-37

@NBNARADHYA
Copy link
Contributor Author

@fanatid @paul-schaaf I used this command while commiting: git commit -m "message" -S. I believe this will sign commits. I am not sure what went wrong. BTW this is the output for git log --show-signature.
Screenshot from 2022-03-09 14-15-09

@NBNARADHYA
Copy link
Contributor Author

@fanatid Ok. Now I figured out I have to use GPG keys to sign. But i tried to edit those commits by rebasing but it edits all other commits and signs them all for some reason

@NBNARADHYA NBNARADHYA force-pushed the cli-workspace branch 3 times, most recently from eeb30ff to c5eec84 Compare March 9, 2022 09:06
@NBNARADHYA NBNARADHYA force-pushed the cli-workspace branch 6 times, most recently from eeb30ff to c5eec84 Compare March 9, 2022 17:28
@paul-schaaf
Copy link
Contributor

we've turned off the signing requirement. Just make sure you have no more conflicts and then Ill merge

@NBNARADHYA
Copy link
Contributor Author

@paul-schaaf Great! Thanks. Just resolved the conflicts

@paul-schaaf paul-schaaf merged commit 00488b5 into coral-xyz:master Mar 12, 2022
@paul-schaaf
Copy link
Contributor

thank you for the contribution!

@NBNARADHYA
Copy link
Contributor Author

@paul-schaaf Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paul-schaaf paul-schaaf paul-schaaf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cli: anchor idl fetch should work outside workspaces
4 participants
@NBNARADHYA @paul-schaaf @armaniferrante @fanatid