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

feat: [UTP-242] Make subnet replica version available to canisters via management API #2202

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

michael-weigelt
Copy link
Contributor

@michael-weigelt michael-weigelt commented Oct 23, 2024

This PR is a follow-up to #2248

It makes replica_version available via a management canister call as specified here.

@alin-at-dfinity
Copy link
Contributor

FYI, you can create a PR on top of a branch other than master (in this case, I believe mwe/subnet_metrics_exe from #2248 would have been appropriate).

Then the change can be reviewed before said branch is merged to master. And when it is merged, then GitHub automatically rebases and changes the base branch to master.

@michael-weigelt
Copy link
Contributor Author

michael-weigelt commented Oct 25, 2024

@alin-at-dfinity

when it is merged, then GitHub automatically rebases

This I did not know, nice.

@michael-weigelt michael-weigelt changed the base branch from master to mwe/subnet_metrics_exe October 25, 2024 09:18
github-merge-queue bot pushed a commit that referenced this pull request Oct 28, 2024
…lable to execution environment (#2248)

This is an alternative to #2248 and
#2082

This PR passes the running replica version to the execution environment,
from where it can be used [in a future management canister
call.](dfinity/interface-spec#351):
#2202

---------

Co-authored-by: Michael Weigelt <[email protected]>
Base automatically changed from mwe/subnet_metrics_exe to master October 28, 2024 10:38
aterga pushed a commit that referenced this pull request Oct 29, 2024
…lable to execution environment (#2248)

This is an alternative to #2248 and
#2082

This PR passes the running replica version to the execution environment,
from where it can be used [in a future management canister
call.](dfinity/interface-spec#351):
#2202

---------

Co-authored-by: Michael Weigelt <[email protected]>
Copy link
Member

@dsarlis dsarlis left a comment

Choose a reason for hiding this comment

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

I left a comment on the spec PR about the name of this endpoint.

I'd also recommend we add some tests calling this endpoint and confirming it returns the right thing. Also, some test that ensures this is not callable via ingress (you should find examples from other similar endpoints).

rs/execution_environment/src/execution_environment.rs Outdated Show resolved Hide resolved
@dsarlis dsarlis requested a review from mraszyk October 29, 2024 13:08
@dsarlis
Copy link
Member

dsarlis commented Oct 29, 2024

Added also Martin as a reviewer explicitly for spec compliance check.

@michael-weigelt michael-weigelt requested a review from a team as a code owner October 29, 2024 15:30
@github-actions github-actions bot added the @idx label Oct 29, 2024
@marko-k0 marko-k0 self-requested a review October 30, 2024 09:29
This reverts commit 8911282.
Copy link
Contributor

@mraszyk mraszyk left a comment

Choose a reason for hiding this comment

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

LGTM - we may still want to rename from subnet_metrics to subnet_info (to be discussed in the upcoming Interface Specification meeting)

assert!(!replica_version.is_empty());
}

#[test]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe one more test where you try sending a request with the wrong subnet_id?

WasmResult::Reject(err_msg) => panic!("Unexpected reject, expected reply: {}", err_msg),
};
let SubnetInfoResponse { replica_version } = Decode!(&bytes, SubnetInfoResponse).unwrap();
assert!(!replica_version.is_empty());
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the replica version a constant that we could compare against?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants