tetratelabs / tetrate-service-bridge-sandbox

Deploy Tetrate Service Bridge Demo on Azure Kubernetes Service (AKS), Google Kubernetes Engine (GKE) and/or Elastic Kubernetes Service (EKS) using Terraform
Apache License 2.0
12 stars 10 forks source link

gcp: allow to fully manage DNS in the current project #129

Closed nacx closed 1 year ago

nacx commented 2 years ago

This makes it easier to use this repo completely self-contained in its own GCP project.

Instead of always configuring DNS records in the shared project, with this change:

Note that this required introducing some variables to the register_fqdn module, but I tried to make those not be coupled to GCP.

smarunich commented 1 year ago

please take a look to https://github.com/smarunich/tetrate-service-bridge-sandbox/pull/130, I have submitted the PR based on your changes and incorporated the suggested fixes

nacx commented 1 year ago

Looking at that PR, I don't fully understand the delta. what is in this PR that doesn't work? If it is some adjustments, can we merge this, then merge yours on top of that? If something is wrong in this PR I'd appreciate the comments in this one, as a learning exercise, otherwise, can we better merge this, then open a PR just with the improvements?

nacx commented 1 year ago

@smarunich this PR works fine, and I've tested it wait the shared DNS (default), public and private. Can't we merge it and work in an incremental way on the fixes? That makes it easier to iterate on stuff having focused discussions, and having feedback in the PR itself instead of fixes been done in a different one helps the contributors engage and learn :)

smarunich commented 1 year ago

Merged the referenced one, discussed the details to make sure that "caveats" are captured for the education purposes.

nacx commented 1 year ago

Fine, but this process doesn't help at all. It makes it much harder for constibutors to understand the delta, no one looking at this PR and the other will understand the gap, and I don't have a clear idea why this PR was not good enough, even though I explicitly asked for feedback in the PR.

There is a reason why contributors ask for a review: it's not a "hey, take it from here and make all amendments you want somewhere else". If someone wants to contribute., let's engage, otherwise this is a quite poor contributor experience for no reason.

Plus, this got closed and the other PR merged without questions and discussion having been answered, such the following (I see the merged Makefile still passes the entire vars file. A decision made having ignored that comment/discussion) : https://github.com/smarunich/tetrate-service-bridge-sandbox/pull/130#discussion_r1002560613

smarunich commented 1 year ago

i think this is more an exception, than the route i want to promote, I added you as a collaborator to the repo, do you mind opening a PR based on the branch of this repo, so if needed - I can submit the fixes to the branch I have access, so I don't have access to nacx/tetrate-service-bridge-sandbox.. it was easier for me, to manage that way as there were introduced bugs... where I could not deploy a POC environment that was requested, so I took it forward to expedite as I poorly reviewed your previous commit that did affected Azure and AWS environments (i.e. not able to create DNS record...).

Again, I am more than welcome to promote anyone to contribute to it, however I need to keep the quality as well... as we don't have e2es yet in place for all the cloud environments, it does require a manual validation.

The vars file is still required for the gcp dns registration to work, i.e. lookup of the gcp infra details - i have removed the redundant vars for aws and azure as you requested, so I actually listened and have ignored nothing.

Again, I am very welcome to any changes so going ahead the advance/complicated changes that it is easier to explain by addressing (fixing), I would appreciate you using this repo. Again, I honestly take care about this environment off my working hours as reality during working hours I have no time to take care about it :|... so I do take shortcuts but again as more people will get to the repo, I will adopt accordingly as it is not only learning curve for contributors, but for the maintainers too... so I do appreciate your honest feedback and contributions!

nacx commented 1 year ago

No one has asked to be promoted or given any privilege. The only thing being asked is proper feedback on the PR. If a PR introduces bugs and you don't share or discuss, and give feedback (especially when explicitly asked for!) you are not helping.

The vars file is still required for the gcp dns registration to work, i.e. lookup of the gcp infra details

Why is the entire file required? I see it only requiring one variable, the `tsb_mp. Do you see? Everything is merged and I still don't understand certain parts of the changes that affect my PR and your changes. Why are all the vars needed? I'd like to learn.

I don't understand the rush to take shortcuts; this is not a critical project. Whenever you have some time to invest in this, please, take your time to accompany the contributor instead of redoing the work; it's a much better use of the time. And if something is so severely broken a fix can't wait... Just revert the commit and ping the contributor, like in every other normal project.

Again, I am very welcome to any changes so going ahead with the advance/complicated changes that it is easier to explain by addressing (fixing), I would appreciate you using this repo

Unfortunately, this is what it takes. Reviews take time. (Do you imagine one single person taking care of redoing and reworking all PRs in the monorepo instead of sharing feedback? It doesn't make sense) If you want people to collaborate in this repo I'd highly encourage you to not do this and take your time to review. That's the difference between enabling people and gatekeeping, and it is fairer for the contributors.

smarunich commented 1 year ago

Absolutely fair points, and I cannot disagree. Lets get to clear the knowledge points and proceed forward the suggested way.