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

Update nns.rs - use latest II and NNS Dapp #60

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krpeacock
Copy link

As requested by Matthew Grogan, updates II and NNS Dapp for dfx extensions. This will automatically pull the wasm from the release tagged as latest

@krpeacock krpeacock requested review from a team as code owners September 1, 2023 16:53
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.

Let's discuss whether we should instead update both of these to a specific more-recent version. Otherwise any future "latest" version of either of these dependencies will be able to break all past versions of the nns extension that depend on them.

@bitdivine
Copy link
Member

bitdivine commented Sep 4, 2023

Personally I would agree with linking a specific version. We have bots that periodically look for recent code, e.g. updates to dfx, the latest IC repo and so on and if an update is found, the bot opens a PR to update the version. We have CI tests that sometimes break when a version is updated so a human has to go in to make any changes related to the update. I believe that this gives a good balance of stability and having recent versions.

@bitdivine
Copy link
Member

That said, the old URLs are really old, so this PR is an improvement over the status quo. If you have time to have a default version and override, I would recommend that and can help with it. If not, I would accept this PR. But this is not my call.

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.

@krpeacock , would you please update this PR to use specific versions, and we will create follow-up tickets to create workflows to update them?

These might be the specific versions:

  • release-2023-08-28 for internet-identity (corresponds to proposal 124282, latest executed update to II canister)
  • proposal-124328 for nns-dapp (proposal 124328 is latest executed update to nns-dapp canister, and corresponds to prod tag today)

@dfx-json
Copy link
Contributor

bump for @krpeacock

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.

4 participants