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

Add workflow file through CLI #64

Merged
merged 8 commits into from
Jan 23, 2024
Merged

Add workflow file through CLI #64

merged 8 commits into from
Jan 23, 2024

Conversation

neelasha23
Copy link
Contributor

@neelasha23 neelasha23 commented Jan 16, 2024

Added instructions on adding Github workflow file through CLI.


📚 Documentation preview 📚: https://ploomber-doc--64.org.readthedocs.build/en/64/

Workflow through CLI

sections

link

link

link

link

link
@neelasha23 neelasha23 marked this pull request as ready for review January 16, 2024 08:25
In order to trigger an action for deploying the project using Github actions you need to add, commit and push this file along with the `ploomber-cloud.json`.

Once done, you can monitor progress as discussed [above](monitor). Ensure that the API key is set as Github secret.
Copy link
Contributor

Choose a reason for hiding this comment

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

add link here to the section that shows how to store the API key using github's UI

Once done, you can monitor progress as discussed [above](monitor). Ensure that the API key is set as Github secret.

In case the [workflow template](https://github.com/edublancas/cloud-template/blob/main/.github/workflows/ploomber-cloud.yaml) has been updated, and you need to re-initialise the application the CLI will prompt your for updating the workflow file:
Copy link
Contributor

Choose a reason for hiding this comment

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

this point is unclear: project re-initialization doesn't happen often. the only case I can think of is if the project gets corrupted.

wouldn't it make more sense to check for an updated workflow file when calling ploomber-cloud deploy?

I see two scenarios:

  1. if ploomber-cloud deploy is running on github actions, then we can juts print a mesage
  2. if ploomber-cloud deploy is running locally, we can deploy and then prompt the user, asking if they want to update their ploomber-cloud.yaml

Copy link
Contributor Author

@neelasha23 neelasha23 Jan 16, 2024

Choose a reason for hiding this comment

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

Yes checking during deploy would make more sense. For scenario 1, the message will be displayed in the github actions logs?

I also had a doubt. Right now there is just a check to see if the repo workflow file contents is different from the template one. This might happen even if user removes the --watch flag from ploomber-cloud deploy --watch or changes the name of the workflow. So do we need to check these specific differences and only then prompt for updation ?

@edublancas

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought through this, here's my feedback (note that this workflow is different to what I proposed earlier cause I noticed some potential issues):

  • we can add a ploomber-cloud github command that can take care of configuring for the first time (if the file doesn't exist) or update the workflow (if it already exists)
  • then, when users run ploomber-cloud init or ploomber-cloud deploy, we can either thell them they can configure github deployment (or update their existing workflow) by running ploomber-cloud github

the reason I'm proposing this is because I feel like prompting users for an answer when running init or deploy might be annoying and unexpected. instead, we can just print some relevant message on init/deploy and they can decide if they want to configure/update github actions by running ploomber-cloud github

Last call is up to you; think what scenarios users will encounter and define the workflow that makes more sense

Copy link
Contributor Author

@neelasha23 neelasha23 Jan 17, 2024

Choose a reason for hiding this comment

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

Sure! I'll start implementing this and if I find any scenario which would be an issue , I'll discuss and modify accordingly.

Also, would it be better to store the last updated date in the workflow file? I haven't found a direct way of doing it in the docs, I still needs to explore if it's possible. Because of reasons I mentioned in the above comment:

Right now there is just a check to see if the repo workflow file contents is different from the template one. This might happen even if user removes the --watch flag from ploomber-cloud deploy --watch or changes the name of the workflow. So do we need to check these specific differences and only then prompt for updation ?

@edublancas

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok to tell the user to update only when the contents are different, no need for more complex implementation right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please refer to this comment

Once done, you can monitor progress as discussed [above](monitor). Ensure that the API key is set as Github secret.

In case the [workflow template](https://github.com/edublancas/cloud-template/blob/main/.github/workflows/ploomber-cloud.yaml) has been updated, and you need to re-initialise the application the CLI will prompt your for updating the workflow file:
Copy link
Contributor

Choose a reason for hiding this comment

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

let's keep spelling consistent:

re-initialise -> re-initialize
github -> GitHub

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added more mentions of GitHub in the latest commit.

@neelasha23 neelasha23 requested a review from edublancas January 22, 2024 15:21
@edublancas
Copy link
Contributor

@neelasha23 don't forget to include responses and links to the observations. it's unclear to me if the were addressed or not

@neelasha23
Copy link
Contributor Author

neelasha23 commented Jan 22, 2024

@neelasha23 don't forget to include responses and links to the observations. it's unclear to me if the were addressed or not

Have addressed all the comments. Most of the code has changed now so I didn't include the links of the previous deleted code. Added links for the ones which are still relevant. @edublancas

@edublancas edublancas merged commit b740256 into main Jan 23, 2024
1 check passed
@edublancas edublancas deleted the workflow branch January 23, 2024 21:30
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.

2 participants