-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
UI: Jobs requested with the wrong namespace under the right conditions #4475
Conversation
Mostly helpful when working with mirage
This was replaced with a relationship at some point.
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.
👍 Cool, I had a click around everywhere and couldn't see any ...'s whereas I could on master. Couple of minor things:
- Your 'unit' tests seem to be testing much more than a single isolated unit, are these more integration tests?
- Might be a good candidate for acceptance tests also?
Actually this has just given me a thought on how I can do some of my PR's better
The job adapter tests started out as unit tests I swear 🙂Just the job adapter itself has grown more and more interdependent. It's the most highly relational model, it extends watchable, and it has namespace voodoo that depends on the system service (which in turn depends on localStorage and the namespaces model/serializer/adapter). I suppose you're right, it's about time to call this what it is.
Yeah, that'd be good. Basically visit the client page for a client with allocations in the default namespace and assert that the correct requests are made. It doesn't add a lot of coverage per se, but it's nice to have something that's a little more e2e. |
hehe yeah I know the type! For acceptance stuff, I was thinking for myself for stuff like this, I might split the failing test off into the first commit of the branch so people can easily see it breaking first, then any commits after that are to fix it. That way its easy to get an idea of what you are looking at whilst you folks are reviewing. I think it would be good for these little bugfix issues, maybe not the bigger features tho. Might do that on my next one, see if it helps. |
Yeah, TDD is great for bug fixes. Some day I'll practice what I preach 🙈 |
😄 I think its more putting the test and the fix in separate commits, I've pretty much been bundling it all up together in the same commit. If the breaking test was in a different commit to the fix it might be helpful for you folks to easily see what its fixing whilst reviewing? |
Maybe? I usually skim the commit messages so it could. If Github had a diff view equivalent of |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
When a namespace is set in localStorage and a job with the namespace
default
is requested, thedefault
namespace gets overridden by the namespace in localStorage.Example:
{ id: 'my-job', namespace: 'default' }
gets requested as/v1/job/my-job?namespace=ns-in-local-storage
Since the jobs routes set the local storage namespace before fetching any data, the only places this could occur were on clients and allocations. The easiest way to reproduce this bug is:
Note that namespaces are an enterprise feature so this ui bug only impacted enterprise deployments.