Skip to content
This repository has been archived by the owner on Nov 4, 2024. It is now read-only.

Add support for metada in createService method #74

Conversation

Oyelowo
Copy link

@Oyelowo Oyelowo commented Feb 17, 2022

Problem

createService method on Deployment class uses deployment name with randomised suffix by default and has no means of defining custom name or namespace for a service. Of course, this can be done with the kx.Service class but it would be nice to provide overriding options in createService method.

This also uses deployment metadata name as service name by default rather than the deployment resource id which provides some stability.

This closes #52 and #73

This also uses deployment metadata name as service name by default rather than the deployment resource id
Copy link
Member

@danielrbradley danielrbradley 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 the contribution!

I've got 1 question inline as I'm not too familiar with the inner workings of this library. I'll see if I can find someone to give this a second review internally too.

return new Service(String(metadata?.name) ?? this.name, {
metadata: {
namespace: this.metadata.namespace,
name: metadata?.name ?? this.metadata.name,
Copy link
Member

Choose a reason for hiding this comment

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

By setting the name here with a default, how will this impact existing users of the library?

@danielrbradley
Copy link
Member

Following an internal conversation, we might not be able to release this immeidately as this component isn't actively supported - meaning some of the release mechanism requires additional updates which we've not got capacity to replace. But if you're happy to leave this open then we'll endevour to incorporate this change at the point we can invest more into this component.

Thanks again for the contribution here, sorry we can't get this shipped out as fast as we'd like.

@mjeffryes
Copy link
Member

Due to lack of tests in this repo and the age, we're not in a good position to move this PR forward so closing for now.

@mjeffryes mjeffryes closed this Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow *.createService to specify metadata
3 participants