-
Notifications
You must be signed in to change notification settings - Fork 92
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 more dapper #670
base: master
Are you sure you want to change the base?
Remove more dapper #670
Conversation
I still see a drone check on this PR, does it need to be rebased? |
@brandond - yeah that's odd, seems like the repo settings may have had Drone PR added back? |
It's at least not blocking the merges any more, so I can go ahead and merge this if you'd like. |
I'll let @thardeck give this a review first and then after that lets merge it. I like the collaboration we're doing and think having them look it over and giving a review too will be good. |
If I click on the branch of this pr it still shows the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would adapt the README.md examples to reflect the removal of the Makefile
.
Besides that looks good to me.
8e5044c
to
9ab0f42
Compare
Ahh - yeah I was still on a stale I'm also looking into the add-tag job issue and will probably make a new PR for that. I will also look into addressing a way to clean up the |
So it seems our failing Are being caused by an upstream k8s issue caused by an upstream (to them) issue with microsoft base images they use; based on this issue: kubernetes-csi/livenessprobe#273 Also just noticed the release note says they cannot build windows images; confused why this caused them to not publish linux images, but likely related. |
Wonder if we should merge this PR or close it? I know some folks still like the uniformity Dapper provides, so maybe we don't remove dapper? I've enjoyed keeping it around in a repo where I need to run the CI scripts locally. I'm not sure if that applies to this repo though, so maybe removing it is fine since we don't need that local use case? |
I am +1 on dropping it from the workflows, but I do think there is value in leaving artifacts in the repo for devs that still use dapper for local testing - at least until we receive direction from management that we are to do away with it entirely. |
53d8665
to
e70befd
Compare
@brandond - this should be good to review now, scoped down to just remove dapper from CI running. However it remains for local dev usage. |
Types of Change
Removes dapper from GHA workflows by calling the scripts directly.
Linked Issues
Follow up for:
#668
#669
Will fix: #666
Additional Notes