-
Notifications
You must be signed in to change notification settings - Fork 200
Remove the assets from dev-scripts #623
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
Conversation
|
I think we can also remove the yq install ref #584 |
|
@bcrochet hey what's the status of this? This will soon be on the critical path as we're pushing PRs to openshift/install so can no longer rely on the dev-scripts hacks. |
|
@hardys openshift/machine-config-operator#795 has merged. This is gtg. |
Thanks! We'll need to merge this along with an update to our pinned release image that includes your changes. We're getting really, really close to where we don't need a custom release image anymore ... |
040acc9 to
109d464
Compare
@russellb I think this has been done, yes? |
|
Testing this PR locally I'm having trouble with it timing out on the image registry. I'm guessing that's because of the assets/templates/99_registry.yaml removal. Since I don't think we migrated that to MCO we may need to leave it. I'm also inclined to think maybe we want to leave assets/templates/99_master-core-password.yaml since that's dev-specific? With those two changes I am able to stand up a cluster. |
|
The registry one is tracked here:
#704
Sent from mobile device
On Aug 6, 2019, at 5:35 PM, Ben Nemec <[email protected]> wrote:
Testing this PR locally I'm having trouble with it timing out on the image
registry. I'm guessing that's because of the
assets/templates/99_registry.yaml removal. Since I don't think we migrated
that to MCO we may need to leave it.
I'm also inclined to think maybe we want to leave
assets/templates/99_master-core-password.yaml since that's dev-specific?
With those two changes I am able to stand up a cluster.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#623?email_source=notifications&email_token=AACLQCWUUDHNXSGAREOIGODQDHVDJA5CNFSM4HYW7X52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3WRENI#issuecomment-518853173>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACLQCTZVUHK2W5OHHEH243QDHVDJANCNFSM4HYW7X5Q>
.
|
|
@bcrochet This still removes the two assets I mentioned above. Do we want to wait this on the image registry change or just leave that asset to be removed separately when the work is complete? I also think we should still leave the core password here as it is useful for development. |
|
Please leave the registry asset. The ingress manifest should not be removed
yet as well
Sent from mobile device
On Aug 7, 2019, at 12:13 PM, Ben Nemec <[email protected]> wrote:
@bcrochet <https://github.com/bcrochet> This still removes the two assets I
mentioned above. Do we want to wait this on the image registry change or
just leave that asset to be removed separately when the work is complete?
I also think we should still leave the core password here as it is useful
for development.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#623?email_source=notifications&email_token=AACLQCURCI6JYUEW7WGPOXDQDLYCXA5CNFSM4HYW7X52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3Y5S3Q#issuecomment-519166318>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACLQCXV4OGP65F7WFOAOYTQDLYCXANCNFSM4HYW7X5Q>
.
|
I must have missed these comments. I'll add them back. |
|
At a minimum I needed these for a successful deployment... If I wasn't using a customised NTP config then I suspect I could have also dropped chrony, but didn't test this as I need it for Ceph/Rook. |
I'm pretty sure that's what's left. |
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1012/ |
|
Yep, that's what I see on the latest version of the PR and it passed my local testing. This should be good to go now. Looks like CI timed out on the worker node. My local run didn't, so I'm guessing that's a heisenbug. |
|
Yea I think that's fine, the cluster came up so I think we're beyond where this affects. LGTM. |
Once these are landed in MCO, we can remove them from dev-scripts