make all alias correctly #7661
make all alias correctly #7661
Conversation
|
Add a simple test
|
Is this preferred in principle? If so, I'll bring it home with unit tests and a list of issues for refactoring. |
|
Yes I'm fine with this change On Fri, Feb 26, 2016 at 4:00 PM, David Eads notifications@github.com
|
|
test and time based cache added. @liggitt ptal. |
| return mapper, api.Scheme | ||
|
|
||
| // at this point we've negotiated and can get the client | ||
| oclient, err := clients.ClientForVersion(nil) |
There was a problem hiding this comment.
This looks like the other methods aren't doing the right thing.
There was a problem hiding this comment.
This looks like the other methods aren't doing the right thing.
I know that this version is pre-negotiated and for a discovery lookup the version doesn't matter so I went with congruence to make the association obvious.
|
Comments addressed. I didn't see a good library method for "replace all illegal characters in a filename" so I went with a very restrictive replacement policy. |
| @@ -450,6 +468,19 @@ func NewFactory(clientConfig kclientcmd.ClientConfig) *Factory { | |||
| return w | |||
| } | |||
|
|
|||
| // overlyCautiousIllegalFileCharacters matches characters that *might* not be supported. Windows is really restrictive, so this is really restrictive | |||
| var overlyCautiousIllegalFileCharacters = regexp.MustCompile(`[^(\w/\.)]`) | |||
There was a problem hiding this comment.
not as cautious as you might hope...
computeDiscoverCacheDir(".","foo/bar") == "foo/bar"
There was a problem hiding this comment.
not as cautious as you might hope...
computeDiscoverCacheDir(".","foo/bar") == "foo/bar"
and? Slash gets used as a path demiliter and you get a sane directory structure right?
There was a problem hiding this comment.
assumed you wanted it to "do a simple collapse of non-AZ09 characters"
|
Not directly related to this PR, but could the caching here be used in other places where we display lists of the available server resources, like |
The information is necessary, but not sufficient. You'd need to harvest all available resources, then find a way to associate those resources potential RESTMapper shortcuts. Currently there's not a structured way to do this. After that, you'd need to inspect the result to see which ones conflict and require qualification. You'd then be left deciding when winning based on priority requires qualification or not. All that work is probably logically required, but I don't think all the APIs are present right now. |
| } | ||
|
|
||
| func (d *CachedDiscoveryClient) getCachedFile(filename string) ([]byte, error) { | ||
| // don't fail on errors, we either don't have a file or won't be able to run the cached check. Either way we can fallback. |
There was a problem hiding this comment.
this comment doesn't make sense here. Maybe add it as a godoc for this function if you want it around. You already include it everywhere getCachedFile is called
|
I'll update the comment and the logging request from @Kargakis . Any additional comments? |
|
Comments addressed. |
| return nil, err | ||
| } | ||
|
|
||
| return cachedBytes, nil |
There was a problem hiding this comment.
I'd like to be able to debug when the client is actually using a cached version of the resources list, or from rest (and then caching it). I'd suggest a couple glog (level 6 probably, like in configs) around the get|writeCachedFile methods.
There was a problem hiding this comment.
I'd like to be able to debug when the client is actually using a cached version of the resources list, or from rest (and then caching it). I'd suggest a couple glog (level 6 probably, like in configs) around the get|writeCachedFile methods.
Done. The write time can be checked in the filesystem and doesn't really affect anything until its read again.
|
Minor comment, but LGTM. Good job on moving shortcuts and aliases out of the factory file. |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/1845/) (Image: devenv-rhel7_3615) |
|
Evaluated for origin merge up to 39ea086 |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to 39ea086 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/1845/) |
This updates the
allalias to actually mean what we want with respect to available resources. There is refactoring needed upstream for purity, but we can delay that . This doesn't change behavior of clients against old servers, so our client looks just as ugly if you're hitting an old atomic server. This doesn't use a cache, but you can see where to plug it.@smarterclayton @fabianofranz @liggitt
Alternative to #7634