-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
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.
looks good overall, just some minor comments
Gopkg.toml
Outdated
|
||
|
||
[[constraint]] | ||
branch = "master" |
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.
shouldn't we lock the constraint to a specific version here?
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.
+1 to Xavier's comment
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.
Good catch. We actually don't need it for now. Cleaned it up in redhat-developer/git-service@8469bca
Gopkg.toml
Outdated
name = "k8s.io/apiserver" | ||
|
||
[[constraint]] | ||
branch = "master" |
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.
same question about the 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.
@oc login -u system:admin | ||
@echo "Creating sub resources..." | ||
@echo "Creating CRDs..." | ||
@oc create -f https://raw.githubusercontent.com/redhat-developer/devopsconsole-operator/master/deploy/crds/devopsconsole_v1alpha1_gitsource_crd.yaml |
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.
or we could include the redhat-developer/devopsconsole-operator/deploy/crds
package as a required dependency (using dep
) and create the CRD from the file in the vendor
directory. It could bring more stability compared to always applying the latest version from master
branch on GitHub.
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 like the idea. But it looks like we can't require a dependency with no go code.
See golang/dep#1306
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.
ah right. As a workaround, we could have a doc.go
file in that package, maybe?
make/Makefile.dev
Outdated
|
||
.PHONY: deploy-apiserver-only | ||
deploy-apiserver-only: | ||
@echo "Creating API Server..." |
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 does not do much, does it?
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.
Correct. Just a placeholder for 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.
no worries, that's what I thought ;)
make/Makefile.dev
Outdated
|
||
.PHONY: create-cr | ||
create-cr: | ||
@echo "Creating Custom Resource..." |
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.
what else is it supposed to do?
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.
It was a placeholder but we probably won't need it anyway. Removed it in redhat-developer/git-service@c31c486
make/test.mk
Outdated
|
||
# Generate HTML representation (and show in browser) of coverage profile (based on unit tests). | ||
# Re-runs unit tests if coverage information is outdated. | ||
.PHONY: coverage-unit-html |
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.
do we need the HTML output? (I've never used it, personally.)
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.
Removed in redhat-developer/git-service@f75c256
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.
thanks!
minishift/README.md
Outdated
|
||
[oc 3.11.0](https://docs.okd.io/latest/cli_reference/get_started_cli.html#installing-the-cli) | ||
|
||
[KVM Hypervisor](https://www.linux-kvm.org/page/Downloads) |
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.
only for Linux
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.
Added a comment in redhat-developer/git-service@7e27a92
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.
thanks
minishift version | ||
``` | ||
|
||
#### Install oc |
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.
not needed thanks to eval $(minishift oc-env)
for those you use Minishift
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.
Changed in redhat-developer/git-service@7e27a92
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.
thanks
minishift/README.md
Outdated
|
||
After running this, you can verify all containers running container inside minishift using `docker ps`. | ||
|
||
**Note**: If you miss this, docker daemon inside minishift couldn't find latest built image which causes `ImagePullBackOff` as we are using `ImagePullPolicy: IfNotPresent` |
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.
couldn't find latest built image
-> will not find the latest image, causing an ImagePullBackOff error ...
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.
Fixed in redhat-developer/git-service@7e27a92
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.
thanks
func main() { | ||
// Placeholder | ||
for { | ||
fmt.Println("Busy doing something super important...") |
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.
definitely! 😂
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.
Looks good to me, just a slight concern about versioning for dependencies.
Gopkg.toml
Outdated
|
||
|
||
[[constraint]] | ||
branch = "master" |
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.
+1 to Xavier's comment
Since it's unclear atm if we still want to go with API Aggregation approach I've extracted the makefile & minishift setup from redhat-developer/git-service#1 to a separate PR (this one).