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

Change Execution Attributes #994

Merged
merged 16 commits into from
May 30, 2019
Merged

Change Execution Attributes #994

merged 16 commits into from
May 30, 2019

Conversation

krhubert
Copy link
Contributor

Changes mentioned in https://forum.mesg.com/t/execution-db-and-api/280

  • rename id to hash
  • remove exeuction duration
  • replace service struct with hash
  • support parent hash

@krhubert krhubert requested review from NicolasMahe, ilgooz and antho1404 and removed request for NicolasMahe and ilgooz May 29, 2019 06:34
execution/execution.go Outdated Show resolved Hide resolved
@antho1404
Copy link
Member

The pull request is doing too many things:

  • There are modifications on the volume key
  • There are update on the proto API with some stuff that I'm not sure even compile
  • Also, the part to change the type in byte might be better in another PR.

protobuf/api/api.proto Outdated Show resolved Hide resolved
protobuf/api/api.proto Outdated Show resolved Hide resolved
@NicolasMahe
Copy link
Member

NicolasMahe commented May 30, 2019

The pull request is doing too many things:

  • There are modifications on the volume key
  • There are update on the proto API with some stuff that I'm not sure even compile
  • Also, the part to change the type in byte might be better in another PR.

I agree with @antho1404. This PR should be splitted into a few more specific ones.
Eg:

  • Rename executionID to executionHash
  • Change executionHash type from string to []byte
  • Add parentHash
  • etc...

EDIT

After checking commit by commit, there is only 2 commits (with 2 or 3 others that fix those) that are blocking the whole PR.. If you have done small PRs, 70% of this PR could have already been merged.

"github.com/stretchr/testify/require"
)

var (
Copy link
Member

Choose a reason for hiding this comment

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

@krhubert krhubert requested a review from antho1404 May 30, 2019 07:27
@antho1404 antho1404 merged commit addd2e9 into dev May 30, 2019
@antho1404 antho1404 deleted the feature/execurion-db branch May 30, 2019 08:16
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