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 PERIAN Job Platform Agent example #1746

Merged
merged 1 commit into from
Oct 17, 2024
Merged

Conversation

otarabai
Copy link
Contributor

@otarabai otarabai commented Oct 9, 2024

The PERIAN agent is already merged here.

Copy link
Contributor

@neverett neverett left a comment

Choose a reason for hiding this comment

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

I left a few suggestions that you can accept as you see fit, but there are two changes you'll need to make to get the docs to build correctly and make tests pass:

  • Update the LABEL format in the Dockerfile
  • Add an entry to the toctree at the bottom of the integrations/index.md file

examples/perian_agent/Dockerfile Outdated Show resolved Hide resolved
examples/perian_agent/README.md Outdated Show resolved Hide resolved
examples/perian_agent/README.md Outdated Show resolved Hide resolved
docs/integrations/index.md Outdated Show resolved Hide resolved
@otarabai
Copy link
Contributor Author

Thanks a lot for the suggestions @neverett. I've applied them all and also updated the example_template.

@otarabai otarabai requested a review from neverett October 10, 2024 08:46
@neverett
Copy link
Contributor

@otarabai This looks great, and thanks for updating the template! Looks like the DCO check is failing, would you be able to sign off on your commits?

@otarabai
Copy link
Contributor Author

otarabai commented Oct 10, 2024

@neverett My mistake. Just signed them off.

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. @otarabai could you help fix the merge conflict?

@neverett
Copy link
Contributor

neverett commented Oct 14, 2024

@otarabai one other thing -- could you add flytekit documentation for this agent? You will need to add a perian.rst page to the flytekit/docs/source/plugins directory (you can check the other plugin pages to see how to format the page), and update the plugins index page with an entry for the Perian agent. Feel free to tag me on that PR when you open it and let me know if you have any questions. Thanks!

@otarabai otarabai force-pushed the master branch 3 times, most recently from 459dbaf to 9433ade Compare October 15, 2024 12:45
@otarabai otarabai requested a review from pingsutw October 15, 2024 12:46
@otarabai
Copy link
Contributor Author

Thanks @pingsutw! I rebased and fixed the conflicts.
@neverett I'll add the flytekit docs right away :)

@otarabai
Copy link
Contributor Author

...and fixed the lint error

@neverett neverett merged commit c8fa39e into flyteorg:master Oct 17, 2024
60 checks passed
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