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

chore: deprecate dfx wallet redeem-faucet-coupon #3514

Conversation

smallstepman
Copy link
Contributor

@smallstepman smallstepman commented Jan 18, 2024

Description

Upcoming PR will introduce functionality allowing for redeeming the coupon straight into cycles-ledger (no wallet involved), making wallet part of the command a bit confusing.

Additionally, we wish to steer people away from dfx wallet unless they really want to use one.

How Has This Been Tested?

covered by CI

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@smallstepman smallstepman requested review from chenyan-dfinity and a team as code owners January 18, 2024 13:32
@smallstepman smallstepman force-pushed the mnl/move_dfx-wallet-redeem-faucet-coupon_command_to_dfx-cycles-redeem-faucet-coupon branch 3 times, most recently from 9e3ce89 to 5919398 Compare January 18, 2024 13:45
Copy link
Member

@ericswanson-dfinity ericswanson-dfinity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great to notice that a command's location isn't going to make sense after some upcoming change, and to move it accordingly.

In the past, when we've removed a command, we've deprecated it first, and then removed it in a later version. This makes it so developers can upgrade, and will have time to make changes after noticing a change like this.

What would you think about doing the same here?

@smallstepman
Copy link
Contributor Author

Absolutely, that makes complete sense. In the meantime, the same has been discussed on Slack with @dfx-json & @sesi200; the current plan is to:

@smallstepman smallstepman force-pushed the mnl/move_dfx-wallet-redeem-faucet-coupon_command_to_dfx-cycles-redeem-faucet-coupon branch from 5919398 to 0059e8d Compare January 23, 2024 14:35
@smallstepman smallstepman changed the title refactor: move command dfx wallet redeem-faucet-coupon to dfx cycles ... chore: deprecate dfx wallet redeem-faucet-coupon Jan 23, 2024
@@ -48,6 +48,9 @@ enum SubCommand {
Custodians(custodians::CustodiansOpts),
Deauthorize(deauthorize::DeauthorizeOpts),
Name(name::NameOpts),
#[deprecated(
note = "This command is deprecated and will be removed in a future version. Eventually, the wallet canister will become an dfx extension."

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should say what command they should use instead. The deprecation warning doesn't need to mention the intention to move the functionality to an extension.

Also, we should display the deprecation warning at runtime. As far as I can tell, this is only displayed at compile time.

Suggested change
note = "This command is deprecated and will be removed in a future version. Eventually, the wallet canister will become an dfx extension."
note = "This command is deprecated and will be removed in a future version. Use XXXXXX instead."
$ dfx-wip wallet redeem-faucet-coupon abc
WARN: Trying to redeem a wallet coupon on a local replica. Did you forget to use '--network ic'?
Redeeming coupon. This may take up to 30 seconds...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right... 🤦

Copy link
Member

@ericswanson-dfinity ericswanson-dfinity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changelog should also mention the deprecation

Copy link
Member

@ericswanson-dfinity ericswanson-dfinity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I would recommend putting this deprecation warning message in the PR that introduces the command that replaces it.

The goal is that someone who sees the deprecation warning can act on it, by replacing their call to the old command with a call to the new command.

@sesi200
Copy link
Contributor

sesi200 commented Feb 12, 2024

Closing in favor of #3569

@sesi200 sesi200 closed this Feb 12, 2024
@sesi200 sesi200 deleted the mnl/move_dfx-wallet-redeem-faucet-coupon_command_to_dfx-cycles-redeem-faucet-coupon branch February 12, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants