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

Fix NPE during ova import #773

Merged
merged 1 commit into from
Dec 8, 2022
Merged

Conversation

shubhaOracle
Copy link
Contributor

Title - Fix NPE during ova import
Issue - During ova import, if event node is null, engine log has error as well as NPE
Fix - Check eventNode for the null value

Signed-off-by: Shubha Kulkarni shubha.kulkarni@oracle.com

Signed-off-by: Shubha Kulkarni <shubha.kulkarni@oracle.com>
@ahadas
Copy link
Member

ahadas commented Dec 7, 2022

/ost

@shubhaOracle
Copy link
Contributor Author

/ost

I see that OST is canceled. How do make sure this runs or fixed?

@michalskrivanek
Copy link
Member

huh, no idea. there ahs been some connectivity issues today. let me try again

@michalskrivanek
Copy link
Member

/ost

@michalskrivanek
Copy link
Member

there seem to be a bug in the check...let me take a look

@michalskrivanek
Copy link
Member

not a bug, arik's fault. for non-members of ovirt organization the exact commit to run needs to be approved. but the last force push invalidated the approval.

@michalskrivanek
Copy link
Member

@ahadas please avoid approving and rebasing, do it the other way around please

@michalskrivanek
Copy link
Member

/ost

@ahadas
Copy link
Member

ahadas commented Dec 7, 2022

@ahadas please avoid approving and rebasing, do it the other way around please

it feels like you give me a car that cannot turn left and say "please avoid turning left, go only straight and right" :)
I guess that unexpected behavior is a remaining of the effort of automatic gating, right?
it's good to know about this limitation but assuming we'll keep the current state of manual merging, can we rather change that to check whether the PR was approved?
@michalskrivanek

@michalskrivanek
Copy link
Member

@ahadas please avoid approving and rebasing, do it the other way around please

it feels like you give me a car that cannot turn left and say "please avoid turning left, go only straight and right" :)

...or fix your car. Patches are welcome.

I guess that unexpected behavior is a remaining of the effort of automatic gating, right?
it's good to know about this limitation but assuming we'll keep the current state of manual merging, can we rather change that to check whether the PR was approved? @michalskrivanek

no, protection of our build infrastructure so that we do not run random code from malicious PRs.
That's also the reason why it is per commit rather than per PR.

@ahadas
Copy link
Member

ahadas commented Dec 8, 2022

no, protection of our build infrastructure so that we do not run random code from malicious PRs. That's also the reason why it is per commit rather than per PR.

Yes, but that was needed when we thought about that automatic gating - OST would run automatically on the PR and once it passes and the PR gets approved, it would be merged by some github action.. now that we trigger OST manually and only people that are part of the oVirt org can do that, it's not an issue, no?

@michalskrivanek
Copy link
Member

no, protection of our build infrastructure so that we do not run random code from malicious PRs. That's also the reason why it is per commit rather than per PR.

Yes, but that was needed when we thought about that automatic gating - OST would run automatically on the PR and once it passes and the PR gets approved, it would be merged by some github action.. now that we trigger OST manually and only people that are part of the oVirt org can do that, it's not an issue, no?

it still is

@ahadas
Copy link
Member

ahadas commented Dec 8, 2022

Yes, but that was needed when we thought about that automatic gating - OST would run automatically on the PR and once it passes and the PR gets approved, it would be merged by some github action.. now that we trigger OST manually and only people that are part of the oVirt org can do that, it's not an issue, no?

it still is

I'm afraid I'll need more information on this if you really welcome patches to address this issue :)

Anyway, let's get this PR in

@ahadas ahadas merged commit 34baab7 into oVirt:master Dec 8, 2022
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

3 participants