modernization #8
Conversation
adding more apt packages for ease of use upgrade pip inline install aws cli for ease of use remove use of non-maintained pygresql in favor of more common/more maintained psycopg2 changed airflow entries to match airflow 2.x new syntax upgraded to cdk v2 upgraded all imports to the new cdk v2 structure upgraded rds to T3 instance class from T2 upgraded typescript to 4.0.5 changing version naming standard to match airflow version + python version loosened strict typscript compiler to allow for dynamic variables fixed some deprecated cdk calls/references added ssh into fargate option
|
Hi @cloneluke, thank you for your contribution! Introducing Airflow v2.x is a really exciting idea I think! To get this PR merged, I think we need some decisions from the authors (@realvz and @thechaithanya) because this repository is, AFAIK, deeply tied to our blog post here - "Running Airflow on AWS Fargate | Containers". So just introducing Airflow v2 here could break the relations between this repo and the blog post. So one idea in my mind is:
But again we still need some decisions (and also some tests) by the authors. @realvz and @thechaithanya, Could you give us your thoughts on this PR and my idea above? Of course different ideas are also very welcomed if you have any 🤗 |
|
Hi @toricls, I'll post my comments on the CR changes later. |
|
Great, thanks @thechaithanya! I just added you as a collaborator in this repo (this is wired in the first place from my view because you're one of the original author 😉), so let me know if the permissions are not enough or anything else! |
Thanks. I haven't configured 2FA earlier. So, my permissions were revoked!! |
| @@ -94,6 +95,6 @@ export class RDSConstruct extends Construct { | |||
| endpoint: string, | |||
| password: string | |||
| ): string { | |||
| return `postgresql+pygresql://${dbConfig.masterUsername}:${password}@${endpoint}:${dbConfig.port}/${dbConfig.dbName}`; | |||
| return `postgresql+psycopg2://${dbConfig.masterUsername}:${password}@${endpoint}:${dbConfig.port}/${dbConfig.dbName}`; | |||
There was a problem hiding this comment.
What's the motive behind this change? Are there any added benefits, as this is used only for metadata?
There was a problem hiding this comment.
psycopg is much more active open source project than pygresql https://github.com/psycopg https://github.com/PyGreSQL so that is why the change was made
| python3-setuptools \ | ||
| libpq-dev \ | ||
| gcc \ | ||
| build-essential \ | ||
| g++ \ | ||
| git-all \ | ||
| unixodbc-dev \ | ||
| apt-utils \ | ||
| apt-transport-https \ | ||
| debconf-utils \ | ||
| vim \ | ||
| telnet |
There was a problem hiding this comment.
Instead of bundling in too many packages, consider moving them to optional requirements.txt, which user can fill in.
If needed, update ReadMe with details
There was a problem hiding this comment.
Isn't requirements.txt a Python thing?
There was a problem hiding this comment.
Yes. And all these packages are being installed using pip.
I should have included Line #11. It looks like this..
RUN python3 -m pip install PyGreSQL argcomplete pycurl
python3-setuptools \
.....
| @@ -1,9 +1,12 @@ | |||
| import { Construct } from "@aws-cdk/core"; | |||
| //import { Construct } from "aws-cdk-lib/core"; | |||
There was a problem hiding this comment.
Delete commented code from this file and other files as well
Also maybe we should use tagging instead of branching for airflow major releases? |
|
Will this PR be merged? |
upgrading to airflow 2.2.2-python3.8 package
adding more apt packages for ease of use
upgrade pip inline
install aws cli for ease of use
remove use of non-maintained pygresql in favor of more common/more maintained psycopg2
changed airflow entries to match airflow 2.x new syntax
upgraded to cdk v2
upgraded all imports to the new cdk v2 structure
upgraded rds to T3 instance class from T2
upgrading postgres rds engine to VER_13_4
upgraded typescript to 4.0.5
changing version naming standard to match airflow version + python version
loosened strict typscript compiler to allow for dynamic variables
fixed some deprecated cdk calls/references
added ssh into fargate option