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

RD-WATCH/Smartflow Integration #412

Merged
merged 22 commits into from
May 31, 2024

Conversation

mvandenburgh
Copy link
Contributor

This PR adds some configuration needed to get RD-WATCH to communicate with SmartFlow. The bulk of the needed changes are done in the infrastructure repo (https://smartgitlab.com/kitware/rgd_aws_infrastructure/-/merge_requests/11), while this PR will just add some initial configuration to demonstrate a proof-of-concept DAG kicked off from RD-WATCH.

@mvandenburgh mvandenburgh force-pushed the smartflow-integration branch 3 times, most recently from 145ffec to 29acbbf Compare May 3, 2024 04:03
@mvandenburgh mvandenburgh force-pushed the smartflow-integration branch 5 times, most recently from 8062dd5 to 199b2cf Compare May 29, 2024 17:22
@mvandenburgh mvandenburgh changed the title [draft] RD-WATCH/Smartflow Integration RD-WATCH/Smartflow Integration May 29, 2024
@mvandenburgh mvandenburgh marked this pull request as ready for review May 29, 2024 18:32
Copy link
Contributor

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

You can feel free to ignore most of the comments, the only things I think may be necessary before merging are:

  • The cd.yaml update to make sure it creates the image
  • The healthcheck port change
  • Not Sure as to why the :latest tag didn't work for me but possibly pinning it to prevent future breaking changes.

dev/airflow.Dockerfile Outdated Show resolved Hide resolved
docker-compose.yaml Outdated Show resolved Hide resolved
vue/src/client/services/SmartflowService.ts Show resolved Hide resolved
vue/src/views/SmartFlow.vue Show resolved Hide resolved
.github/workflows/cd.yaml Outdated Show resolved Hide resolved
Comment on lines +6 to +7
--username rdwatch \
--password rdwatch \
Copy link
Contributor

Choose a reason for hiding this comment

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

this is hardcoded so in the .env file should we make a note of this? It isn't like other containers where the username/password is parameterized. Or maybe we should use the airflow environment variables?

https://airflow.apache.org/docs/apache-airflow/stable/howto/docker-compose/index.html#environment-variables-supported-by-docker-compose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw this, but I couldn't get it to work so I just gave up and did that command in the docker file. Does it work for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

took a little searching but I think you can remove the local airflow DockerFile. These _ based commands are only meant for local development, but I assume your infrastructure code handles all this.
I wouldn't worry about implementation now, just logging this comment for the future.

  airflow:
    image: apache/airflow:slim-latest-python3.12
    volumes:
      - airflow-data:/opt/airflow
      - type: bind
        source: ${PWD}/dev/airflow_sample_dag.py
        target: /opt/airflow/dags/airflow_sample_dag.py
    environment:
      AIRFLOW_HOME: "/opt/airflow/"
      _AIRFLOW_DB_MIGRATE: True
      _AIRFLOW_WWW_USER_CREATE: True
      _AIRFLOW_WWW_USER_USERNAME: "rdwatch"
      _AIRFLOW_WWW_USER_PASSWORD: "rdwatch"
    command: ["airflow", "standalone"]
    ports:
      - 8002:8080
    healthcheck:
      test: ["CMD", "curl", "--fail", "http://localhost:8080/health"]
      interval: 30s
      timeout: 10s
      retries: 5
      start_period: 30s

vue/src/views/SmartFlow.vue Outdated Show resolved Hide resolved
vue/src/views/SmartFlow.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

I figured out the default password/username for airflow when running locally which could allow us to remove the airflow.Docker but I wouldn't worry about it for now. I just added a comment for future reference.

@mvandenburgh mvandenburgh merged commit d01d293 into ResonantGeoData:main May 31, 2024
8 checks passed
@mvandenburgh mvandenburgh deleted the smartflow-integration branch May 31, 2024 13:02
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