Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix local_dev.md file for updated file path & environment variable name #236

Conversation

Priyankasaggu11929
Copy link
Contributor

@Priyankasaggu11929 Priyankasaggu11929 commented Nov 11, 2021

What this PR does / why we need it:

The PR makes the following changes:

  • delete the interim cluster-template-byoh.yaml template file.

  • fix the docs/local_dev.md file to reflect above file changes, in the following command under the section create-docker-hosts:

    cat test/e2e/data/infrastructure-provider-byoh/v1beta1/cluster-templat.yaml | envsubst | kubectl apply -f -
    

The PR does the following small fixes in the local_dev.md document:

  • Fix the file path from test/e2e/data/infrastructure-provider-bringyourownhost/v1beta1/cluster-template-byoh.yaml to the updated test/e2e/data/infrastructure-provider-byoh/v1beta1/cluster-template-byoh.yaml
  • Update the environment variable name from CONTROL_PLANE_ENDPOINT_IP to CONTROL_PLANE_ENDPOINT

Additional information:

Before the changes, while following the steps listed in the document (under create docker hosts section), I was getting the following errors:

  • First error, due to wrong file path:

    ❯ cat test/e2e/data/infrastructure-provider-bringyourownhost/v1beta1/cluster-template-byoh.yaml | envsubst | kubectl apply -f -
    cat: test/e2e/data/infrastructure-provider-bringyourownhost/v1beta1/cluster-template-byoh.yaml: No such file or directory
    error: no objects passed to apply
    
  • After fixing the file path, the second error due to wrong environment variable name (or not matching environment variable name available to fill the value of host field in the cluster-template-byoh.yaml file, so ultimately giving empty host value)

❯ cat test/e2e/data/infrastructure-provider-byoh/v1beta1/cluster-template-byoh.yaml | envsubst |  kubectl apply -f -
cluster.cluster.x-k8s.io/test1 unchanged
error: error validating "STDIN": error validating data: ValidationError(ByoCluster.spec.controlPlaneEndpoint): missing required field "host" in io.x-k8s.cluster.infrastructure.v1beta1.ByoCluster.spec.controlPlaneEndpoint; if you choose to ignore these errors, turn validation off with --validate=false

@dharmjit
Copy link
Contributor

Hi @Priyankasaggu11929, Thank you for validating the local dev guide and raising this PR.
We can update the variable name to CONTROL_PLANE_ENDPOINT_IP in cluster-template-byoh as we are using the same name in other templates.

@dharmjit dharmjit requested a review from a team November 12, 2021 07:08
@anusha94
Copy link
Contributor

We can update the variable name to CONTROL_PLANE_ENDPOINT_IP in cluster-template-byoh as we are using the same name in other templates.

Another suggestion: cluster-template-byoh.yaml was an interim template until cluster-template.yaml was ready. In a subsequent PR, we can delete cluster-template-byoh. So one option would be to use cluster-template.yaml for workload cluster creation which I believe is using CONTROL_PLANE_ENDPOINT_IP variable

@Priyankasaggu11929
Copy link
Contributor Author

Hello @anusha94 & @dharmjit, thanks so much for the review & feedback.


Another suggestion: cluster-template-byoh.yaml was an interim template until cluster-template.yaml was ready. In a subsequent PR, we can delete cluster-template-byoh. So one option would be to use cluster-template.yaml for workload cluster creation which I believe is using CONTROL_PLANE_ENDPOINT_IP variable

One follow up question on above.

So, IIUC, implementing the above suggestion would mean:

  • deleting the cluster-template-byoh.yaml file
  • And updating the following command in the local_dev.md file with a reference to cluster-template.yaml in place of cluster-template-byoh.yaml ?
cat test/e2e/data/infrastructure-provider-byoh/v1beta1/cluster-template-byoh.yaml | envsubst |  kubectl apply -f -

I can make the above changes in this PR only or a subsequent PR is required for the file deletion?

Thank you so much, once again!

@anusha94
Copy link
Contributor

@Priyankasaggu11929

I can make the above changes in this PR only or a subsequent PR is required for the file deletion?

Yes, your understanding is correct. I don't have any strong opinion. Please go ahead and delete in this PR itself if that's easier.

@shamsher31
Copy link
Contributor

shamsher31 commented Nov 18, 2021

/cc @dharmjit

@github-actions github-actions bot requested review from dharmjit and removed request for a team November 18, 2021 05:13
@shamsher31
Copy link
Contributor

/assign @anusha94

@Priyankasaggu11929
Copy link
Contributor Author

Thanks @anusha94. I'll update the PR shortly. (apologies for the late response, just involved in my new Hire Corporate Orientation sessions). 🙂

@Priyankasaggu11929
Copy link
Contributor Author

@anusha94, I've updated the PR. Thank you!

Copy link
Contributor

@dharmjit dharmjit left a comment

Choose a reason for hiding this comment

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

Congrats @Priyankasaggu11929 for your first contribution to BYOH.

LGTM!

@anusha94 anusha94 merged commit 42b6981 into vmware-tanzu:main Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants