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

Lcaggio/df cx #1768

Closed
wants to merge 3 commits into from
Closed

Lcaggio/df cx #1768

wants to merge 3 commits into from

Conversation

lcaggio
Copy link
Collaborator

@lcaggio lcaggio commented Oct 17, 2023

Create a Dialgoflow CX agent with private connectivity to a Cloud Run instance acting as webhook.


I applicable, I acknowledge that I have:

  • Read the contributing guide
  • Ran terraform fmt on all modified files
  • Regenerated the relevant README.md files using tools/tfdoc.py
  • Made sure all relevant tests pass

# When deploying to Google App Engine, a webserver process such as
# Gunicorn will serve the app. This can be configured by adding an
# `entrypoint` to app.yaml.
app.run(host='127.0.0.1', port=8080, debug=True)

Check failure

Code scanning / CodeQL

Flask app is run in debug mode High

A Flask app appears to be run in debug mode. This may allow an attacker to run arbitrary code through the debugger.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lately I'm leaning against PRs that add code in other languages because their original author usually merges and never looks back to make sure everything works.

This warning is one more reason to avoid this kind of assets.

Copy link
Collaborator

@juliocc juliocc left a comment

Choose a reason for hiding this comment

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

@lcaggio every blueprint we merge represents an additional maintenance burden on the repo maintainers. Please understand that we need to balance whether a new blueprint is a meaningful addition to the repository against the limited maintenance resources we have.

I know this WIP but I'm struggling to see the value of this blueprint. I'd really love to have more ML-focused blueprints but I have to question if there's anything novel in this PR.

--region=${var.region} \
--project=${module.project.project_id} \
--tag ${var.region}-docker.pkg.dev/${module.project.project_id}/${google_artifact_registry_repository.repo.name}/webhook:v1 \
./cr
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a big no. We never shell out to run random commands. Our only dependency is the GCP provider and nothing else

# When deploying to Google App Engine, a webserver process such as
# Gunicorn will serve the app. This can be configured by adding an
# `entrypoint` to app.yaml.
app.run(host='127.0.0.1', port=8080, debug=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lately I'm leaning against PRs that add code in other languages because their original author usually merges and never looks back to make sure everything works.

This warning is one more reason to avoid this kind of assets.


# tfdoc:file:description Cloud Run resources.

module "service_account_cr" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we use dashes in module names

@@ -0,0 +1,14 @@
# GCP ML/AI Services blueprints

The blueprints in this folder implement **typical ML/AI service topologies** and **end-to-end scenarios**, that allow testing specific features.
Copy link
Collaborator

Choose a reason for hiding this comment

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

that allow testing specific features this is not the purpose of blueprints

provisioner "local-exec" {
working_dir = path.module
command = "openssl x509 -in server_tf.crt -out server_tf.der -outform DER"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as before. This will never be merged as it stands.

@ludoo
Copy link
Collaborator

ludoo commented Oct 17, 2023

Hey Lore, I agree 100% on what Julio wrote. I am closing this, let's figure out together if it makes sense to bring this further, and how to avoid contradicting several of our design principles. :)

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.

None yet

3 participants