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

feat: adding a host parameter to the target server to be able to use DNS #152

Closed
wants to merge 1 commit into from

Conversation

rs1986x
Copy link
Contributor

@rs1986x rs1986x commented Apr 22, 2024

What's changed, or what was fixed?

I have added the possibility of specifying a host parameter for target server. i will use this in combination with https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/google_service_networking_peered_dns_domain to access load balancer with a url mask type serverless backend with one single psc

Fixes: #issue

  • I have run all the tests locally and they all pass.
  • I have followed the relevant style guide for my changes.

@rs1986x rs1986x force-pushed the allow-external-ip-target-server branch from 6f36ae5 to 099d1cb Compare April 22, 2024 13:18
@danistrebel
Copy link
Collaborator

danistrebel commented Apr 22, 2024

Hi @rs1986x, thanks for the PR.

I'm wondering what's the motivation for using the sb-psc module in the first place if you already have a host that you want to use. wouldn't you be better off by just using the google_apigee_target_server resource directly?

The creation of the other two resources looks wasteful at first glance. Maybe I'm missing something here.

@rs1986x
Copy link
Contributor Author

rs1986x commented Apr 22, 2024

Very good question @danistrebel: i run all my terraform with terragrunt, which has many advantages, with the exception that it really works well if you put all of your code in module form. i was faced with 2 choices here:

  1. what i did
  2. write a different module to manage the target servers independently.

honestly either approach is fine, with method 1 i manage all in one module, with method 2 i have the advantage of easier migration for the target servers (if i need to change the endpoint attachement for a particular target server)

tl;dr - the code is sound, the "is this a good change for this module" is totally up for debate

@danistrebel
Copy link
Collaborator

Thanks for the background.

Since the module is really meant to provide SB PSC functionality, using it for creating only an Apigee target server seems a bit of an overly complex approach. Given this is literally just about creating a module wrapper around a single resource I'd vote to keep the module as is.

Maybe it's better for your use case to fork this and have an Apigee southbound connection module that supports for PSC and direct host names.

@danistrebel
Copy link
Collaborator

Closing this, appreciate the suggestion and the constructive discussion here!

@danistrebel danistrebel closed this May 3, 2024
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.

None yet

2 participants