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 SelfLink #106

Merged
merged 10 commits into from
Apr 7, 2021
Merged

Remove SelfLink #106

merged 10 commits into from
Apr 7, 2021

Conversation

dhaiducek
Copy link
Contributor

I wasn't 100% sure why we were using selfLink here, so I've replaced it with the UID since I assumed we wanted some sort of unique identifier. Otherwise, we may need to reconstruct selfLink ourselves if we indeed do need it.

@openshift-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@gparvin
Copy link
Contributor

gparvin commented Apr 6, 2021

The self link was originally there so the UI could provide the link in the list of related resources.
Example: For a non compliant policy, click the "Status" and then click "View details" for any template that is Not Compliant. The table at the bottom is the related resources table and the "View yaml" link seems to work using fields other than the selfLink... so I'm agreeing here that selfLink can be removed unless someone knows of some other use case that depends on it.

@dhaiducek
Copy link
Contributor Author

The UI constructs a SelfLink for its own uses, so maybe even the UID isn't necessary here? (Though I suppose it helps to have some sort of identifier...I haven't really dug into the code too much beyond replacing the instances with the UID)

@dhaiducek dhaiducek requested review from gparvin and ycao56 April 6, 2021 14:20
@dhaiducek
Copy link
Contributor Author

@ycao56 What are your thoughts on this?

@dhaiducek dhaiducek marked this pull request as ready for review April 6, 2021 14:21
@dhaiducek
Copy link
Contributor Author

/hold

@ycao56
Copy link

ycao56 commented Apr 6, 2021

I think we can remove it both selflink and uid to see if we need to change the UI. I couldn't remember if we updated the UI or not.

@ycao56
Copy link

ycao56 commented Apr 6, 2021

BTW, I don't see selfLink being requested by UI anymore so we must have changed it previously due to ocp47 support
image

@dhaiducek
Copy link
Contributor Author

dhaiducek commented Apr 6, 2021

Do we want to remove my implementation of UID also, or is it helpful? I think it could be a simple array of resource names--I'd have to dig into the code a little deeper to be sure.

@ycao56
Copy link

ycao56 commented Apr 6, 2021

I think we should completely remove it

@dhaiducek
Copy link
Contributor Author

Done--ready for review! If these checks pass, I'll update the branch protection rules in release for the updated KinD version so that the PR can merge.

Fixes error:
```
failed to start the controlplane. retried 5 times: fork/exec /usr/local/kubebuilder/bin/etcd: no such file or directory
```
Reference: operator-framework/operator-sdk#4432 (comment)
@ycao56
Copy link

ycao56 commented Apr 6, 2021

@dhaiducek also need to update CRD https://github.com/open-cluster-management/config-policy-controller/blob/main/deploy/crds/policy.open-cluster-management.io_configurationpolicies_crd.yaml#L152
run operator-sdk generate k8s and operator-sdk generate crds
You may have to downgrade to an older version of operator-sdk as this project was generated using v0.17.0

@dhaiducek
Copy link
Contributor Author

Ah, okay--that's probably the issue I'm running into. Thanks!

@dhaiducek
Copy link
Contributor Author

I bumped my operator-sdk down to v0.17.0. It created a policies.policy.open-cluster-management.io CRD. Should that also be merged here? I'm assuming not since it wasn't there before...

@ycao56
Copy link

ycao56 commented Apr 6, 2021

can you also generate v1 version and put it in https://github.com/open-cluster-management/config-policy-controller/tree/main/deploy/crds/v1?
There is a flag that you can set operator-sdk generate crds --crd-version v1

@ycao56
Copy link

ycao56 commented Apr 6, 2021

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, ycao56

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ycao56
Copy link

ycao56 commented Apr 6, 2021

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 6, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dhaiducek
Copy link
Contributor Author

/unhold

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants