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

Refs #18288 - Add missing Notification dependency #6966

Merged
merged 1 commit into from Oct 3, 2017

Conversation

jturel
Copy link
Member

@jturel jturel commented Sep 21, 2017

@waldenraines This one's for you! Saw some errors in the js console and other odd behavior ... simple fixes.

@theforeman-bot
Copy link

Issues: #18288

@jturel
Copy link
Member Author

jturel commented Sep 21, 2017

[test]

@waldenraines waldenraines self-assigned this Sep 21, 2017
@waldenraines
Copy link
Contributor

@jturel thanks! I'm not sure if @ehelms requires a new bug in this case since the code for #18288 has already been merged.

@jlsherrill
Copy link
Member

I'm fine including it in the existing issue, since it completes that work.

@ehelms
Copy link
Member

ehelms commented Sep 21, 2017

This has the potential to be missed by our tooling given the originating issue was already marked against a release that has been branched and is being cherry picked.

@jlsherrill
Copy link
Member

if cherry picks have been done already, would a new issue actually help? Wouldn't our tooling pick up on the new commit regardless?

Copy link
Contributor

@waldenraines waldenraines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me, not sure how I missed them but thanks @jturel!

@waldenraines
Copy link
Contributor

This has the potential to be missed by our tooling given the originating issue was already marked against a release that has been branched and is being cherry picked.

if cherry picks have been done already, would a new issue actually help? Wouldn't our tooling pick up on the new commit regardless?

I don't really care either way so I'll let y'all decide before merging this.

@jturel
Copy link
Member Author

jturel commented Sep 25, 2017

@jlsherrill @ehelms - shall I open a new issue or not? I personally feel like it shouldn't be necessary but I see valid points above.

@ehelms
Copy link
Member

ehelms commented Oct 3, 2017

You can leave it as is, but, to avoid issues I would generally always recommend 1:1 correspondence of PR to Redmine especially after branching.

@waldenraines
Copy link
Contributor

You can leave it as is, but, to avoid issues I would generally always recommend 1:1 correspondence of PR to Redmine especially after branching.

Let's consider this a test to see what happens if you merge without a new issue 😄

@waldenraines waldenraines merged commit e95b949 into Katello:master Oct 3, 2017
@jturel jturel deleted the 18288 branch August 20, 2021 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants