Skip to content

Generalise Modules Service to make it extensible#2559

Merged
pstibrany merged 15 commits intocortexproject:masterfrom
annanay25:refactor-svcs
May 15, 2020
Merged

Generalise Modules Service to make it extensible#2559
pstibrany merged 15 commits intocortexproject:masterfrom
annanay25:refactor-svcs

Conversation

@annanay25
Copy link
Contributor

@annanay25 annanay25 commented May 5, 2020

Signed-off-by: Annanay annanayagarwal@gmail.com

What this PR does:
Implementation of the generalise module service design doc.

Which issue(s) this PR fixes:
Fixes #2291

Checklist

  • Tests updated
  • NA Documentation added
  • NA CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Annanay <annanayagarwal@gmail.com>
annanay25 added 3 commits May 5, 2020 18:37
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@pstibrany pstibrany self-requested a review May 6, 2020 09:52
@jtlisi jtlisi self-requested a review May 6, 2020 12:43
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thanks for your work! I've left some comments, please take a look.

Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

Once the rename PR is merged I will give this another pass.

annanay25 added 4 commits May 8, 2020 12:24
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

This looks close to ready, just needs some cleanup to make sure CICD is passing.

Signed-off-by: Annanay <annanayagarwal@gmail.com>
@pstibrany
Copy link
Contributor

This looks close to ready, just needs some cleanup to make sure CICD is passing.

This PR also changes wrapping part (all modules are wrapped), so it cannot be merged until Server module can work with that.

@annanay25
Copy link
Contributor Author

@pstibrany - Looks like your changes to signal handling on weaveworks/common were merged. I will push the final changes here now.

@pstibrany
Copy link
Contributor

This PR also changes wrapping part (all modules are wrapped), so it cannot be merged until Server module can work with that.

#2576 implements this.

annanay25 added 6 commits May 11, 2020 19:05
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

assert.NotNil(t, svcs)
assert.NoError(t, err)

svcs, err = mm.InitModuleServices("service_unknown")
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is interesting because it calls InitModuleServices twice... it's not clear from documentation what should actually happen in that case!

Would you mind adding a test for missing dependency too?

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM. I've left a small comment about test, but we can always improve that later.

@pstibrany pstibrany merged commit aa10e35 into cortexproject:master May 15, 2020
@annanay25
Copy link
Contributor Author

Thanks! I'll send a follow up PR for the test.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generalize Module Service

3 participants