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

Improvements to mock MutableData implementation #826

Merged
merged 12 commits into from Jun 6, 2019

Conversation

Projects
None yet
3 participants
@lionel1704
Copy link
Member

commented Jun 4, 2019

  • Improve mock impl. for MData by introducing a local enum in the mock vault impl.
  • Add MsgIdConverter trait
  • Add functions to the Client trait for other mutable data operations
    with relevant tests

lionel1704 added some commits May 31, 2019

refactor/mdata: Refactor code to use proper structs
- Use proper structs for mutable data
- Write separate tests to put and get both sequenced and unsequenced mutable data
feat/safe_core: Improve mdata impl in mock-vault and handle more RPCs
- Improve mock impl. for MData by using the MutableData enum
- Add functions to the `Client` trait for other mutable data operations
with relevant tests

@lionel1704 lionel1704 force-pushed the lionel1704:mdata-improv branch from 138a025 to c032e62 Jun 4, 2019

@lionel1704 lionel1704 marked this pull request as ready for review Jun 5, 2019

@lionel1704 lionel1704 requested a review from nbaksalyar as a code owner Jun 5, 2019

@lionel1704

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Closing and reopening to trigger Travis CI

@lionel1704 lionel1704 closed this Jun 5, 2019

@lionel1704 lionel1704 reopened this Jun 5, 2019

refactor/safe_core: adapt to changes in safe-nd
- Introduce local enum for MData variants in mock vault impl.
- Add MsgIdConverter trait
- Handle get_mdata_shell separately for seq. and unseq. mdata
- Modify tests to work with the changes

@lionel1704 lionel1704 force-pushed the lionel1704:mdata-improv branch from db5e9f4 to 59074b1 Jun 5, 2019

refactor/mdata: adapt to changes in safe-nd
- Use the `Requester` enum for MData RPCs
- Temporarily handle permissions only for `Requester::Key`
@nbaksalyar
Copy link
Member

left a comment

Looks good, thanks! 👍 Just a couple of comments from me.

Could you please also cherry-pick the @jacderida's branch from here to make the Jenkins CI work?
#829

@@ -130,37 +132,38 @@ impl Vault {
}
}

pub fn authorised_read(
pub fn verify_requester(

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jun 6, 2019

Member

Better to have this function in safe-nd, but it's OK to leave it here for now.

@@ -231,32 +234,205 @@ impl Vault {
dest: Authority<XorName>,
payload: Vec<u8>,
) -> Result<(Authority<XorName>, Vec<u8>), ClientError> {
let request = unwrap!(deserialise(&payload));
let request: Request = unwrap!(deserialise(&payload));
dbg!(request.clone());

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jun 6, 2019

Member

I think we don't need that? 🙂

};
let result_buffer = unwrap!(res);
let res: Response<ClientError> = unwrap!(deserialise(&result_buffer));
dbg!(res.clone());

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jun 6, 2019

Member

Another one dbg! we need to remove (alternatively, convert it into trace! log call)

};
let result_buffer = unwrap!(res);
let res: Response<ClientError> = unwrap!(deserialise(&result_buffer));
dbg!(res.clone());

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jun 6, 2019

Member

One more. And there are some more after this one - could you pls also remove them?

lionel1704 and others added some commits Jun 6, 2019

refactor/build: define params in jenkinsfile
For a multibranch pipeline job, you can't define parameters on the job
itself using the job DSL plugin, so they need to be defined here.

It doesn't look like there's actually much advantage of declaring them
this way as opposed to just using normal variables for them, but it
could be they need to be set this way for chaining builds in the future.
chore/ci: update rust version for container
Updates the container's version of Rust to 1.34.1, which is the latest
stable version. Unfortunately Rust don't add a 'stable' tag for the
containers they publish, so this version number will need to evolve
along with whatever version is stable. Unless at some point they produce
a 'stable' tag.
chore/ci: use variables rather than props
Using those properties was the suggested way for having parameters for a
multi branch pipeline job, but it doesn't seem to work. Going to just
set these as normal variables for now.
fix/ci: clone code on each stage
For a distributed setup where a stage can run on a different machine,
you need to re-clone the code.
chore/ci: use branch name in artifacts
The build number on its own isn't enough for Multibranch Pipeline jobs.
We need to set the branch name based on whether the CHANGE_ID property
is set. This will be set to the PR reference for PR builds and not set
on branch builds, so we'll just use the branch name for those.
chore/ci: conditional for deployment
Introduces a conditional so that deployment will only occur on a certain
branch. I tried to do this originally by defining the deployment branch
in a parameter, just in case we ever wanted to change that, but doing
the comparison just didn't seem to work correctly. I've tested that this
setup works correctly.

@lionel1704 lionel1704 force-pushed the lionel1704:mdata-improv branch from 76048be to ff73337 Jun 6, 2019

@lionel1704 lionel1704 requested a review from nbaksalyar Jun 6, 2019

@nbaksalyar
Copy link
Member

left a comment

Looks great! 🚀

@nbaksalyar nbaksalyar merged commit 915d20c into maidsafe:experimental Jun 6, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@lionel1704 lionel1704 deleted the lionel1704:mdata-improv branch Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.