-
Notifications
You must be signed in to change notification settings - Fork 86
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
Create CI infrastructure in GitHub Actions #337
Conversation
@kensipe, do you want me to remove the |
oh.. nice! We may want to give a heads up and align with kudo... but would limit this to < a week. I don't see any challenges but want to allow for alignment time. and to answer the question.. yes! we should remove Nice stuff! |
Ok, nice. |
it would be much nicer to have releases off github actions. |
I see all the details are there either on the release doc.. or linked from it. |
this is marked "Draft"... let me know when you're ready. It looks good. I expect we want to pull from circleci... is that the only thing left to change? I would suggest that we can have additional features (like the release process) on a separate PR. Thanks for this! |
@iblancasa checking to see if this is ready? |
@iblancasa do you think that is ready? |
renewed interest in this... I can take it over if needed |
@kensipe I can finish it if you want. It seems @eddycharly started something in #423 |
I didn't know about this PR, I made something super simple (only runs the linter and unit tests). |
Ok, this PR was originally to create the releases too. How about just fixing the PRs and create another PR for the releases? |
@iblancasa your PR looks great, are we going to merge it ? |
@eddycharly if everyone agrees, we can merge it. |
Outch all is red ! |
This would be great to have ! |
@eddycharly I'm trying to fix it. It'll take a while... |
Signed-off-by: Israel Blancas <iblancasa@gmail.com>
@kensipe @eddycharly ready for review. Sorry because I messed up something with a merge and I had to do some pushes... |
|
||
sudo mkdir -p /usr/local/kubebuilder/bin | ||
|
||
sudo curl -L https://dl.k8s.io/v1.16.4/kubernetes-server-linux-amd64.tar.gz -o /usr/local/kubebuilder/bin/kubernetes-server-linux-amd64.tar.gz |
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.
This uses k8s 1.16 ?
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'll guess so. I suggest installing etcd and apiserver using https://github.com/kubernetes-sigs/controller-runtime/tree/master/tools/setup-envtest in a follow-up MR - that can also upgrade to a supported K8s version.
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'm not sure. I ported what the previous automation had. I think we can do the upgrade later as @erikgb suggested
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.
agree with the team that we should upgrade the k8s version... but aligned with iblancasa that we can do that on the next PR
very interested in this PR and those related. I will be reviewing today and pushing to merge asap |
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.
In general LGTM, but I would prefer if the setup task were better integration into the Makefile, to allow it to be run on local developer workstation. But that can be improved in a follow-up PR. As mentioned in a comment, we also need to upgrade the K8s version. K8s 1.16 is so totally unsupported.
I also think we should remove the .circleci
, as having multiple CI configs will appear confusing.
I agree about the upgrades and everything but, as I pointed in this comment, #337 (comment) this PRs is just porting the CI from one system to another. |
agree with @erikgb however I'm good with landing this and removing circle in a separate PR. reviewing... however we will likely want to review the actual experience. Reviewing content now. |
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.
/lgtm. thank you so much @iblancasa ! I will be merging today. Based on tests, I will create a PR to remove circle. Following that we can look to bump versions to something more modern.
|
||
sudo mkdir -p /usr/local/kubebuilder/bin | ||
|
||
sudo curl -L https://dl.k8s.io/v1.16.4/kubernetes-server-linux-amd64.tar.gz -o /usr/local/kubebuilder/bin/kubernetes-server-linux-amd64.tar.gz |
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.
agree with the team that we should upgrade the k8s version... but aligned with iblancasa that we can do that on the next PR
More importantly I think we should swith to kind or something closer to a real cluster. |
@eddycharly we use api-server and etc for integration tests... we use |
where does this happen ? |
In the e2e logs:
|
Signed-off-by: Israel Blancas iblancas@redhat.com
What this PR does / why we need it:
Some notes