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

Refactor cosmos modules #1650

Merged
merged 8 commits into from
Feb 18, 2020
Merged

Refactor cosmos modules #1650

merged 8 commits into from
Feb 18, 2020

Conversation

krhubert
Copy link
Contributor

@krhubert krhubert commented Feb 10, 2020

related to #1639

TODO:

  • use one function in reset/cli querier
  • Add cosmos wrap error to all module errors

@krhubert krhubert self-assigned this Feb 10, 2020
@krhubert krhubert force-pushed the refactor/cosmos-modules branch from 84c0a48 to bc2b445 Compare February 15, 2020 19:35
@krhubert krhubert changed the title [WIP/SHOWCASE] Cosmosization of the codebase Refactor of cosmos modules Feb 15, 2020
@krhubert krhubert requested review from NicolasMahe and antho1404 and removed request for NicolasMahe February 15, 2020 19:36
@krhubert krhubert added this to the next milestone Feb 15, 2020
- create cosmos module client
- removed sdk packages
- create runner/builder and event/publisher packages
- remove orchestrator tests & mocks
@krhubert krhubert force-pushed the refactor/cosmos-modules branch from 5a06ec3 to b9288be Compare February 17, 2020 07:02
@krhubert krhubert marked this pull request as ready for review February 17, 2020 12:17

// Create creates a new execution.
func (s *SDK) Create(req *api.CreateExecutionRequest) (*executionpb.Execution, error) {
execution.M.Created.Add(1)
Copy link
Member

Choose a reason for hiding this comment

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

no more metrics?

Copy link
Member

Choose a reason for hiding this comment

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

could be nice to have a define a shared metric object and use it across the engine
but then it's against the client / module separation

Copy link
Member

@NicolasMahe NicolasMahe Feb 18, 2020

Choose a reason for hiding this comment

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

i'm ok to merge without this 👍 @krhubert i let you merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should be right im the execution. Chęci keeper się. Unfournetly i coundnt find a good place for them. We can add that to refactor list to keep track of that

@NicolasMahe NicolasMahe added the release:change Pull requests that change something existant label Feb 18, 2020
@NicolasMahe NicolasMahe merged commit e691c81 into dev Feb 18, 2020
@NicolasMahe NicolasMahe deleted the refactor/cosmos-modules branch February 18, 2020 06:57
@NicolasMahe NicolasMahe changed the title Refactor of cosmos modules Refactor cosmos modules Mar 2, 2020
@NicolasMahe NicolasMahe mentioned this pull request Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring release:change Pull requests that change something existant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants