Skip to content

Conversation

@pkosiec
Copy link
Collaborator

@pkosiec pkosiec commented Aug 19, 2021

Description

Changes proposed in this pull request:

  • Update syntax for injecting required Type Instances

    • Fetch TypeInstance metadata in PolicyEnforcedClient based just on ID
    • Add optional description field for a given requiredTypeInstances field
    • rename inject.typeInstances to inject.requiredTypeInstances

    Elegant syntax comparison table (inspired by this PR):

    Before After
     inject:
       requiredTypeInstances:
         - id: uuid
           typeRef: // wasn't validated anywhere
     	  path: "cap.type.sample"
     	  revision: "0.1.0" // optional
     inject:
       requiredTypeInstances:
       - id: uuid
         description: "My UUID" // optional
  • remove apiVersion from Policy (as Policy is strictly versioned with Engine component, as the Policy shape is defined in GraphQL API)

  • Fix printing null for Policy management commands in CLI

To do (in a follow-up pull request):

  • Ignore Implementations with requires.alias when TypeInstances are not injected

Testing

Engine

No additional testing needed as TypeInstance injection is covered by integration tests. See the test setup Global Policy for changes.

CLI

Run:

# create cluster
DISABLE_MONITORING_INSTALLATION=true make dev-cluster

# build cli
make build-tool-cli
mv ./bin/capact-darwin-amd64 /usr/local/bin/capact

# test cli
capact policy get
capact policy edit

Related issue(s)

#438

@pkosiec pkosiec added enhancement New feature or request area/engine Relates to Engine area/hub Relates to Hub labels Aug 19, 2021
@pkosiec pkosiec marked this pull request as ready for review August 20, 2021 08:36
@pkosiec pkosiec added the WIP Work in progress label Aug 20, 2021
@pkosiec pkosiec removed the WIP Work in progress label Aug 20, 2021
@pkosiec pkosiec force-pushed the required-ti branch 3 times, most recently from fc70259 to 73ac57d Compare August 23, 2021 07:18
@pkosiec pkosiec added the breaking Contains breaking change label Aug 23, 2021
@pkosiec pkosiec force-pushed the required-ti branch 2 times, most recently from 86dcf11 to a699b9b Compare August 23, 2021 12:31
Copy link
Collaborator

@mszostok mszostok left a comment

Choose a reason for hiding this comment

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

We need to decided how we will merge that. I see quite a lot of places where we will have conflicts with #450 😄 We need to reserver some time for that.

BTW Nice test coverage 👍

return string(bytes), nil
}

// HubClient defines Hub client which is able to find TypeInstance Type references.
Copy link
Collaborator

Choose a reason for hiding this comment

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

tbh I really don't like that those functions landed in types.go. IMO in types.go we should have types definition and max. simple functions associated with a given types, just helpers. As described in PR

Fetch TypeInstance metadata in PolicyEnforcedClient based just on ID

Personally, I think that it will be more readable in my opinion. I know that here it's tightly coupled with given Type which is a plus and it can be used as a part of API but for me it's a bit messy when Types, "business logic" and validation is in single file.

But of course this is just academic discussion and definitely I will not block that from being merged as you did a great job. Maybe later we can discuss our approach in dev team.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed, I will convert the ValidateTypeInstanceMetadata method to a function in a separate package. I will keep validation methods as they are right now.

This change will be introduced in a follow-up pull request, which is #458.

Comment on lines +159 to +161
out = append(out, &graphql.RequiredTypeInstanceReference{
ID: item.ID,
Description: item.Description,
Copy link
Collaborator

Choose a reason for hiding this comment

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

WDYT about resolving that TypeRef already here?

Currently after applying:

cat > /tmp/aws-policy.yaml << ENDOFFILE
rules:
  - interface:
      path: cap.interface.containerization.kubernetes.deploy
    oneOf:
      - implementationConstraints:
          path: cap.implementation.aws.containerization.rke2.deploy
        inject:
          requiredTypeInstances:
          - id: "e7f9f8ee-2217-4704-af34-2610727ec567"
            typeRef:
              path: "cap.type.aws.auth.credentials"
              revision: "0.1.0"

  - interface:
      path: cap.*
    oneOf:
      - implementationConstraints:
          requires:
            - path: "cap.core.type.platform.kubernetes"
      - implementationConstraints: {}
ENDOFFILE

capact policy apply -f /tmp/aws-policy.yaml

We get:

$ capact pol get
rules:
- interface:
    path: cap.interface.containerization.kubernetes.deploy
  oneOf:
  - implementationConstraints:
      path: cap.implementation.aws.containerization.rke2.deploy
    inject:
      requiredTypeInstances:
      - id: e7f9f8ee-2217-4704-af34-2610727ec567
- interface:
    path: cap.*
  oneOf:
  - implementationConstraints:
      requires:
      - path: cap.core.type.platform.kubernetes
  - implementationConstraints: {}

but IMO it will be nice to see the typeRef too, as we resolve it anyway

@pkosiec pkosiec merged commit 658845d into capactio:main Aug 24, 2021
@pkosiec pkosiec deleted the required-ti branch August 24, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/engine Relates to Engine area/hub Relates to Hub breaking Contains breaking change enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants