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

cleanup action test #16

Merged
merged 8 commits into from
Nov 11, 2022
Merged

cleanup action test #16

merged 8 commits into from
Nov 11, 2022

Conversation

aakankshaduggal
Copy link
Member

Quick cleanup for action files.

  • Get access to all env variables in action.yml
  • Run a test for the inference notebook for an incoming PR

@github-actions
Copy link

Our Model Predicts this PR to be in category [6]

@aakankshaduggal aakankshaduggal changed the title Aakanksha action test cleanup action test Nov 11, 2022
entrypoint.sh Outdated Show resolved Hide resolved
Co-authored-by: Michael Clifford <mcliffor@redhat.com>
@MichaelClifford
Copy link
Collaborator

Can you also update inference.yaml to remove all the steps and replace it with

    steps:
      - name: Run Model Inference
        uses: redhat-et/time-to-merge-tool@v1

@github-actions
Copy link

Our Model Predicts this PR to be in category [6]

name: Time To Merge Tool - Model Inference Test
description: 'This is the github action to predict time to merge for a new pull request'
author: 'redhat-et'

inputs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not 100% sure how we handle the env variables, but I don't think we need them in this file and the inference action yaml.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at a few existing action.yml files I see most of them are inputting these env variables in this file itself. https://github.com/actions/stale/blob/main/action.yml
However, if you suggest we can keep them in the inference.yaml only and move them to action later?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no no. let's leave them in both places for now. But I think eventually we will be able to move them out of the inference.yaml

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree with this. 👍

action.yml Outdated
outputs:
prediction:
description: 'Provides a prediction of the PRs time to merge'

runs:
using: 'docker'
image: 'Containerfile'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to change the file name from Containerfile to Dockerfile and change the image: tag here to Dockerfile as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, got it

@MichaelClifford
Copy link
Collaborator

/LGTM

@MichaelClifford MichaelClifford merged commit e939737 into main Nov 11, 2022
@aakankshaduggal
Copy link
Member Author

#11

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