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

remove deprecated wps providers (attempt 2) #293

Merged
merged 2 commits into from
Jul 6, 2023
Merged

remove deprecated wps providers (attempt 2) #293

merged 2 commits into from
Jul 6, 2023

Conversation

mishaschwartz
Copy link
Collaborator

This is an addition to #292.

I had mistakenly assumed that notebook outputs would be re-generated if the inputs were updated but since that is not the case, this makes the relevant changes to the output cells as well.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Contributor

@tlvu tlvu left a comment

Choose a reason for hiding this comment

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

LGTM

@mishaschwartz
Copy link
Collaborator Author

@tlvu just checking in on this. I don't have permission to merge changes to this repo if you're waiting on me.

@tlvu
Copy link
Contributor

tlvu commented Jun 14, 2023

@tlvu just checking in on this. I don't have permission to merge changes to this repo if you're waiting on me.

Added you as collaborator to this repo.

If you need more write access to other repos, please do not hesitate.

@mishaschwartz
Copy link
Collaborator Author

@tlvu Can I just merge this or are there additional procedures for this repo before I merge?

@tlvu
Copy link
Contributor

tlvu commented Jun 15, 2023

@tlvu Can I just merge this or are there additional procedures for this repo before I merge?

Run a Jenkins specifying the branch in this PR.

If my understanding of Weaver is right, you can only merge this PR when the Mallefow are actually disabled from the stack. Otherwise this Weaver nb will start failing in all the Pipeline.

@mishaschwartz
Copy link
Collaborator Author

@tlvu

If my understanding of Weaver is right, you can only merge this PR when the Mallefow are actually disabled from the stack. Otherwise this Weaver nb will start failing in all the Pipeline.

Ok but then #292 shouldn't have been merged. Should we revert #292 and wait to re-implement it when bird-house/birdhouse-deploy#311 is merged?

@tlvu
Copy link
Contributor

tlvu commented Jun 19, 2023

@tlvu

If my understanding of Weaver is right, you can only merge this PR when the Mallefow are actually disabled from the stack. Otherwise this Weaver nb will start failing in all the Pipeline.

Ok but then #292 shouldn't have been merged. Should we revert #292 and wait to re-implement it when bird-house/birdhouse-deploy#311 is merged?

Woops ! Is that why Weaver tests are failing now in the pipeline?

How about we do the reverse, we prioritize merging #311? What's blocking it?

I'd say if we can merge #311 within 2-3 weeks, forget about reverting #292.

In the past, Weaver have been failing for months in the pipeline simply because the notebooks was not up-to-date so I think this is not a big deal.

@mishaschwartz
Copy link
Collaborator Author

@tlvu

What's blocking it?

Just waiting for a final review from @fmigneault

@mishaschwartz
Copy link
Collaborator Author

Thanks for approving this one @fmigneault.

In this comment:

What's blocking it?

Just waiting for a final review from @fmigneault

I was actually referring to your review for this PR: bird-house/birdhouse-deploy#311 if you don't mind having another look at that one when you have a minute.

@huard
Copy link
Contributor

huard commented Jul 6, 2023

Is this good to go ?

@huard huard merged commit fdab4ee into Ouranosinc:master Jul 6, 2023
@mishaschwartz mishaschwartz deleted the patch-2 branch July 6, 2023 18:23
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.

4 participants