upbound / function-cidr

Composition Function that transforms CIDRs
Apache License 2.0
4 stars 3 forks source link

improve pipeline support for function-cidr #33

Open anessi opened 1 month ago

anessi commented 1 month ago

Description of your changes

Motivation: the current code does not allow to use multiple function calls in a pipeline, because it only allows to read from observed.composite.resource properties. In pipelines we need to be able to read values from other fields and use them as input value (e.g. prefix).

This PR implements these main changes:

Please see the added example pipeline for a concrete example how I use the function of the use-case.

Fixes #32

The updated function is available for testing as:

apiVersion: pkg.crossplane.io/v1beta1
kind: Function
metadata:
  name: function-cidr
spec:
  package: xpkg.upbound.io/anessi/function-cidr:v0.5.4

Please note that I'm not a GO developer, so review the code carefully as I might have missed some obvious things 😃

I have:

humoflife commented 2 weeks ago

Do you need the backslashes in. apis/composition-pipeline-context.yaml here: prefixField: context.apiextensions\.crossplane\.io/extra-resources.XCluster.0.spec.cidrBlock ? If not, please remove them.

humoflife commented 2 weeks ago

When running make render, rendering cidrsubnetloop and cidrsubnets stopped working. The others are ok.

Please take a look.

Rendering examples/xr-cidrsubnetloop.yaml...
crossplane: error: cannot render composite resource: pipeline step "cidr" returned a fatal result: cannot get output from field spec.parameters.output for XCIDR: spec.parameters.output: no such field
Rendering examples/xr-cidrsubnets.yaml...
crossplane: error: cannot render composite resource: pipeline step "cidr" returned a fatal result: cannot get output from field spec.parameters.output for XCIDR: spec.parameters.output: no such field
make: *** [render] Error 1
humoflife commented 2 weeks ago

function-cidr is offering cidrFunc and cidrFuncField inputs.

The description of the changes includes:

... add consistent field support for cidrFunc (this is a breaking change, but used for consistency reasons: use cidrFuncField instead of cidrFunc now)

What is the breaking change and why should not both cidrFunc and cidrFuncField be supported with their original intent and in analogy to other parameters?

humoflife commented 2 weeks ago

Please add test coverage in fn_test.go.

humoflife commented 2 weeks ago

Was the output field not supported prior to this PR? What was added?

humoflife commented 2 weeks ago

Please run all yaml files through yamlfmt (https://formulae.brew.sh/formula/yamlfmt if you are using Mac OSX) to update the indentation of arrays or manually update, e.g.

foo:
  - bar1
  - bar2

versus:

 foo:
 - bar1
 - bae2
humoflife commented 2 weeks ago

In the comment in input/v1beta1/parameters.go, please replace

// cidrfunc is one of cidrhost, cidrnetmast, cidesubnet, cidrsubnets, cidrsubnetloop

with

// cidrfunc is one of cidrhost, cidrnetmast, cidrsubnet, cidrsubnets, cidrsubnetloop
anessi commented 2 weeks ago

Do you need the backslashes in. apis/composition-pipeline-context.yaml here: prefixField: context.apiextensions\.crossplane\.io/extra-resources.XCluster.0.spec.cidrBlock ? If not, please remove them.

They are needed because it's a key with dots in the name (apiextensions.crossplane.io/extra-resources). See https://github.com/crossplane-contrib/function-extra-resources/tree/main

make render

This is fixed now. Thanks for noticing.

cidrFuncField vs cidrFunc

There was no cidrFuncField before, but instead cidrFunc had the same behavior as now cidrFuncField. This was inconsistent and I fixed this to make it consistent. See https://github.com/upbound/function-cidr/pull/33/files#diff-e7b8a7f13b611cc9ad80aad4c3c35c12930ad31ddfb43b721a692c0c8d069391

outputField vs output

output was not supported, but only outputField

yamlfmt

The files are formatted with yamlfmt now.

// cidrfunc is one of cidrhost, cidrnetmast, cidrsubnet, cidrsubnets, cidrsubnetloop

Fixed the comments. There was a second spelling mistake (cidrnetmast).

Please add test coverage in fn_test.go.

Looks like the project does not have any tests so far. What would you like me to do?

Wouldn't it be easier if you add the comments directly on the code instead of different comments here? Makes it a bit hard to give replies to individual comments.