tektoncd / results

Long term storage of execution results.
Apache License 2.0
78 stars 74 forks source link

[CLI] Fix the broken flags #568

Closed khrm closed 1 year ago

khrm commented 1 year ago

Earlier, we weren't calling getconfig in the beginning. After we added port-forward, we added the GetConfig call earlier. This fixed the global flag and when we merge the global flag fix, it broke the local flag. Now, refactored the setting flag/config to singleton in the init to avoid undefined scenarios.

Fixes #567

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you review them:

Release Notes

NONE
khrm commented 1 year ago

@avinal @sayan-biswas @enarha Let's get this merge asap.

enarha commented 1 year ago

/lgtm

avinal commented 1 year ago

Tested! Resolves the issue, but I am noticing a new problem that was not present before.

$ ./tkn-results list default --filter="summary.status=='SUCCESS'"    
ListResults: rpc error: code = InvalidArgument desc = error compiling CEL filters: ERROR: <input>:1:15: found no matching overload for '_==_' applied to '(int, string)'
 | summary.status=='SUCCESS'
 | ..............^
Error: rpc error: code = InvalidArgument desc = error compiling CEL filters: ERROR: <input>:1:15: found no matching overload for '_==_' applied to '(int, string)'
 | summary.status=='SUCCESS'
 | ..............^

I am uncertain if this is related, if someone else is also getting the same error, this will need to be fixed as well.

khrm commented 1 year ago

@avinal

Can you test with these commits: https://github.com/tektoncd/results/commit/3643dcc5356f467d447c2cc6c567bc9f4569c98a https://github.com/tektoncd/results/commit/89ee88eb8fdc9e767af3bfab0367ff175ef6b5c9

Is that error present in both of the revision?

avinal commented 1 year ago

Yes, it is present in both of them.

avinal commented 1 year ago

Ah sorry I was just checking the tkn-results, I need to check by re deploying the whole project. Let me do that.

Update: no luck similar results.

khrm commented 1 year ago

Is it happening with only this filter or every filter? @avinal

khrm commented 1 year ago

I created another issue regarding this. https://github.com/tektoncd/results/issues/569 This pr doesn't cause this issue and it should be tackled separately.

avinal commented 1 year ago

Only happening with this particular filter expression. All other I tested are working fine. It seems somehow the summary.status field is interpreted as int.

avinal commented 1 year ago

We are good to go with this PR then. /approve

enarha commented 1 year ago

/approve on behalf of @avinal

tekton-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avinal, enarha

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: - ~~[OWNERS](https://github.com/tektoncd/results/blob/main/OWNERS)~~ [enarha] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment