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

Run CI in AWS EC2 instance #103

Merged
merged 3 commits into from
Mar 1, 2023
Merged

Run CI in AWS EC2 instance #103

merged 3 commits into from
Mar 1, 2023

Conversation

misohu
Copy link
Member

@misohu misohu commented Feb 23, 2023

  1. Change CI to execute in AWS - there is no space in the github runner to run the newly created mlflow image
  2. Move the charm code to root folder - MLflow 2.1.1. will be a standalone charm no point to store code in charms subfolder
  3. Remove examples folder - as we use 2.1.1. examples will change

The CI uses this action which will create a ec2 instance which was created for the AWS service account (kubeflow-team). All the AWS credentials and GH PAT are part of the secrets (go to settings -> secrets and variables -> actions). All the AWS EC2 configuration is part of the repo variables (go to settings -> secrets and variables -> actions).

CI uses custom AMI created from ubuntu LTS 20.04 AMI and installing docker inside. The AMI is part of the service account private AMI list (its the value set in environment variables).

Publish charm will fail as it is merging agains feature branch not main (but it works on main check this draft pr against main with the same code)

Change CI to execute in AWS
Move the charm code to root folder
Remove examples folder
@misohu misohu requested a review from a team as a code owner February 23, 2023 12:16
@misohu misohu mentioned this pull request Feb 23, 2023
@misohu misohu marked this pull request as draft February 23, 2023 14:09
Change CI to execute in AWS
Move the charm code to root folder
Remove examples folder
@misohu misohu marked this pull request as ready for review February 27, 2023 13:54
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @misohu, few comments.

.github/workflows/integrate.yaml Show resolved Hide resolved
.github/workflows/integrate.yaml Outdated Show resolved Hide resolved
.github/workflows/publish.yaml Show resolved Hide resolved
.github/workflows/setup_environment.sh Show resolved Hide resolved
.github/workflows/setup_environment.sh Outdated Show resolved Hide resolved
.github/workflows/setup_environment.sh Outdated Show resolved Hide resolved
.github/workflows/setup_environment.sh Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@ca-scribner
Copy link
Contributor

Don't refactor the current PR, but for a future thought my first impression here is I see both CI changes (aws stuff) and repo structure changes (moving charm from ./charms/mlflowserver to ./). Those sound like good things to separate into different PRs - of 43 files changed, about 30 of them are file moves. (I might have missed a nuance for this case, but ... ) if the repo structure change is a separate PR that only moves the files plus patches the old CI to work with the new file locations (only minimal changes to keep the old functionality), that's a really easy review because we can see the old functionality is maintained. As is, we have to look at those moves and new CI and decide jointly if everything is ok

Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

Some changes. Also, should integration tests be failing during charm build?

Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

(deleted)

@ca-scribner ca-scribner dismissed their stale review February 28, 2023 21:46

Doesn't need to be blocking

remove juju bundle from setup environment, test on microk8s 1.24, fix typos in comments, fix integration tests
@misohu misohu merged commit e1d97c6 into KF-715-mlflow-v211 Mar 1, 2023
@misohu misohu deleted the KF-715-aws-ci branch March 1, 2023 12:05
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.

3 participants