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.

Sign up for GitHub

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

Jump to bottom

RHOAIENG-905 - Create a PR check for testing README file (without gitops) #191

Merged
merged 4 commits into from
Dec 19, 2023
Merged

RHOAIENG-905 - Create a PR check for testing README file (without gitops) #191

merged 4 commits into from
Dec 19, 2023

Conversation

MarianMacik
Copy link
Member

@MarianMacik MarianMacik commented Dec 13, 2023 •
edited
Loading

…ops)

JIRA: RHOAIENG-905

Description

Adds 2 scripts:

  • common.sh which acts as a library
  • pipeline-test.sh currently testing just the build-container-image-pipeline and test-mlflow-image-pipeline with housing (TensorFlow) version of the pipeline run

How Has This Been Tested?

Locally using a separate cluster.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Dec 13, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Dec 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MarianMacik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@MarianMacik
Copy link
Member Author

/test all

@MarianMacik MarianMacik requested a review from winklerm December 13, 2023 12:56
@MarianMacik
Copy link
Member Author

/retest

@LaVLaS
Copy link
Contributor

LaVLaS commented Dec 13, 2023

@MarianMacik Can you add a link to the Jira issue in the description for anyone who want to check for additional context in the jira issue?

@MarianMacik
Copy link
Member Author

/retest

@MarianMacik
Copy link
Member Author

/retest

@MarianMacik MarianMacik removed the request for review from piotrpdev December 14, 2023 13:47
@MarianMacik
Copy link
Member Author

/retest

@MarianMacik
Copy link
Member Author

/retest

winklerm
winklerm reviewed Dec 15, 2023
View reviewed changes
Makefile
@@ -32,4 +32,6 @@ GO=go
GOFLAGS=""

test:
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed earlier, this target could be split into a target for the Go tests and a target for the shell tests, so they could be both triggered independently, if needed. The test target could call both of them.

This can be done later, just mentioning it here so it does not get forgotten.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, this will be addressed in the next PRs.

Copy link
Contributor

@LaVLaS LaVLaS Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Since this will be executing two separate frameworks, should we go head a make a test-go target to separate them now
Ignore this comment after re-reading the thread

LaVLaS
LaVLaS reviewed Dec 18, 2023
View reviewed changes
test/shell-pipeline-tests/common.sh
local AWS_SECRET_PATH_TEMPLATE=$1
local AWS_SECRET_PATH=$2

local AI_EDGE_AWS_VAULT_OPENSHIFT_CI_SECRET_PATH
Copy link
Contributor

@LaVLaS LaVLaS Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Based on this naming convention, does this require that running this locally requires setting up the secrets in the format that is used with openshift-ci?

Copy link
Member Author

Choose a reason for hiding this comment

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

@LaVLaS yes, you are right. I added the readme to guide what needs to be done. It is just 3 variables which need to be set. In the future, we may even provide some defaults in the Makefile.

@MarianMacik MarianMacik marked this pull request as ready for review December 18, 2023 16:42
@winklerm
Copy link
Contributor

Looks good to me, not approving now in case @LaVLaS has additional comments.

@LaVLaS
Copy link
Contributor

LaVLaS commented Dec 19, 2023

/lgtm

Changes look reasonable to me.

@openshift-ci openshift-ci bot added the lgtm label Dec 19, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 70b248f into opendatahub-io:main Dec 19, 2023
2 checks passed
@adelton adelton mentioned this pull request Dec 20, 2023
Closed
jackdelahunt pushed a commit to jackdelahunt/ai-edge that referenced this pull request Jan 23, 2024
@openshift-merge-bot
Merge pull request opendatahub-io#191 from MarianMacik/RHOAIENG-905
3c60a87 
RHOAIENG-905 - Create a PR check for testing README file (without gitops)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LaVLaS LaVLaS LaVLaS left review comments

@winklerm winklerm winklerm left review comments

Assignees

@LaVLaS LaVLaS

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarianMacik @LaVLaS @winklerm