Skip to content

feat: add ServicePort and NetworkService to framework#200

Draft
Siddhartha-singh01 wants to merge 2 commits into
score-spec:mainfrom
Siddhartha-singh01:feat/shared-provisioner-network-types
Draft

feat: add ServicePort and NetworkService to framework#200
Siddhartha-singh01 wants to merge 2 commits into
score-spec:mainfrom
Siddhartha-singh01:feat/shared-provisioner-network-types

Conversation

@Siddhartha-singh01

Copy link
Copy Markdown

Description

Adds shared framework types for provisioner network connectivity. PR 1 of 10 in the #191 lift.

  • ServicePort resolved port: Name, Port, TargetPort, Protocol
  • NetworkService contact info for another workload: ServiceName, Ports map

What does this PR do?

Introduces types provisioners will store in State.SharedState. Additive only.

Part of #191. Does not wire into provisioners yet (PR 2 + PR 7).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • New chore (expected functionality to be implemented)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've signed off with an email address that matches the commit author.

Signed-off-by: Siddhartha Singh <siddharthagithub0007@gmail.com>
@Siddhartha-singh01

Copy link
Copy Markdown
Author

Hi @mathieu-benoit!

Here is the first PR for #191. As discussed, I'm keeping these PRs strictly scoped and incremental to make reviewing easy.

This PR simply introduces the foundational network types to the framework package so we can start building the SharedState helpers in the next PR.

A few specific design notes on this implementation:

Resolved Types: For ServicePort, I explicitly made TargetPort a required int (rather than a pointer like it is in types.ServicePort). Since this represents a resolved port going into SharedState, enforcing it as a value type will prevent downstream provisioners from having to do nil-pointer checks.

YAML Tag Conventions: I deliberately used snake_case for the YAML tags (e.g., yaml:"target_port", yaml:"service_name") and omitted JSON tags. This strictly aligns with the existing YAML-only serialization conventions you use in framework/state.go (like shared_state and source_workload), rather than porting over the mixed JSON/YAML tags from the types package.

Testing: I added YAML round-trip tests using the idiomatic t.Run() structures found in state_test.go to ensure both TCP and UDP protocols serialize exactly as expected with the correct field names.

Let me know if you are happy with this structure and the YAML tag alignment! Once this is merged, I'll immediately open PR 2 to add the SharedState helper functions that will utilize these types.

@mathieu-benoit

Copy link
Copy Markdown
Contributor

Thanks, @Siddhartha-singh01!

Out of curiosity, would it possible to use this branch directly in dedicated PRs in score-compose and score-k8s to see the integration/impact of this there? Or you still need PR 2 like you mentioned?

@Siddhartha-singh01

Copy link
Copy Markdown
Author

Hii @mathieu-benoit

Great question! We technically could test this branch directly in score-compose and score-k8s, but it wouldn't give you an accurate picture of the final integration. Since this PR only adds the raw types, the provisioners would have to manually deal with messy type-casting from the SharedState map.

PR 2 is what introduces the strongly-typed Get/Set helper functions that score-compose will actually use to make the integration clean. We can either merge this and I'll open PR 2 immediately, or if you prefer, I can push the PR 2 helpers into a temporary branch and use that to open a draft PR in score-compose so you can see the full end-to-end picture. Let me know what you prefer!

Thanks !

@mathieu-benoit

Copy link
Copy Markdown
Contributor

@chris-stephenson, like discussed during the community call, could you please review and advise here before @Siddhartha-singh01 put more effort on this? Thanks1

@Siddhartha-singh01

Copy link
Copy Markdown
Author

@mathieu-benoit @chris-stephenson Got it sir

Looking for the more advise and Review

Thanks !

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.

2 participants