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

RFE: Provider PRs with main repo PR dependency alert gitter channel #357

Closed
NickLaMuro opened this issue Aug 31, 2017 · 8 comments
Closed

Comments

@NickLaMuro
Copy link
Member

NickLaMuro commented Aug 31, 2017

Scenario

I am a developer minding my own business when the following are merged without my knowledge:

I decide I need to run bin/update for some reason (maybe I clobbered my assets, or want a fresh DB). Halfway through, I get the following error:

SupportsFeatureMixin::UnknownFeatureError: Feature ':add_security_group' is unknown to SupportsFeatureMixin.
/Users/gpiatigorski/dev/manageiq/app/models/mixins/supports_feature_mixin.rb:138:in guard_queryable_feature'
/Users/gpiatigorski/dev/manageiq/app/models/mixins/supports_feature_mixin.rb:182:insupports'

This occurred because bin/update in ManageIQ/manageiq will run bundle update, updating the manageiq-providers-openstack gem, but not the dependency in the main repo.

Proposal

This scenario is not so uncommon with how many git gems ManageIQ now employs. A one way to simply avoid this confusion is to post a message in the chat when the provider PR is merged, with contents similar to the following:

ManageIQ/manageiq-providers-openstack#79 has been merged, which will most like cause issues with an outdated ManageIQ/manage master branch. Make sure to fetch upstream and rebase to avoid issues.

Triggering this message could be based off a pending core label existing on the provider PR, but possibly a more elegant solution could be thought up (something in this case wasn't even present on the provider PR).

@chrisarcand
Copy link
Member

Personally I just assume I should be running the bleeding edge of everything to expect things to work. Given that we version nothing (because Git-based gems are all the rage in ManageIQland), the only real expectation one can have is that the current tip of every project aligns properly.

I feel like labeling and notifying about every little thing that could cause an incompatibility between our provider repos, schema, UI, API, etc etc etc will just become such a common occurrence that it will become noise that everyone will ignore eventually anyway.

Such is a consequence of our [current] architecture, for now.


I'm not against doing this, I just don't think it'll be worth our time. If others think it will be, great!

@NickLaMuro
Copy link
Member Author

So just to be clear, this RFE was spawned from me not reporting, but responding to a report of an error:

https://gitter.im/ManageIQ/manageiq?at=59a883a5bac826f054693a00

I admit, I didn't actually say who the target audience was, or rather who would benefit the most, in my PR description (but those who hate reading my 📚 probably appreciate that I didn't). That said, this was more of a selfish suggestion than an altruistic one. Mostly, that I wouldn't have to be dealing with trying to solve this for someone else, especially when the answer is so simple.

I say simple, but not obviously unless you are either:

a) fully up to date and don't happen to be working on something for a long period of time (I do on a frequent basis for performance issues for example)
b) fully aware of everything going on in the entire ManageIQ eco-system, which unless you are someone who doesn't sleep, is probably exactly 0 people

The only way I personally was able to determine the cause is I was able to also reproduce it myself, which provided me with a stacktrace with more context, specifically pointing to a file in the openstack provider. From there, that raised a red flag that I should check for recent changes in the openstack provider repo. I thankfully found the PR near the top, but it lacked any description, and I barely noticed the dependency on a core change until I looked again and saw it in the comments.

Personally I just assume I should be running the bleeding edge of everything to expect things to work.

While this may be your expectation, I don't think this is a common expectation, especially from anyone outside of the organization. If I git clone a project as a new developer, then take a hour to read the documentation while those PRs are merged, when I come back to run bin/update, the expected reaction is probably not going to be "golly gee, guess I am not bleeding edge enough", but probably "this sucks" and either give you or ask for help.


We can also solve this by printing "hey, you are out of date with ManageIQ/manageiq:master" (if they are behind, and are branched-off/on master I guess) after an error happens in bin/update. But again, that falls into the same pitfall of only showing up on the machine of the developer who has run into the error, and if this isn't reproducible by those "who live on the edge", then unless that info is not also ignored, there is a huge chance for lost time as people waste time determine what the cause is.

Yes, this in most cases will be noise, but it will hit the largest audience this way and will make sure both parties (the reporter and responder) are aware of any relevant changes. Mean that in most cases, one of the two will probably happen:

  • The reporter will noticed the message prior asking about it in the main chat, and quickly double check they are fully up to date
  • The issue is still reported, but the responder can see the message in chat, and confirm with the reporter that the root cause is an out of date master, or something else entirely.

@bdunne
Copy link
Member

bdunne commented Sep 1, 2017

I think this would create enough noise that I would disable notifications for any Gitter rooms these messages are sent to.. 😟 Maybe bin/update could be enhanced instead?

@jrafanie
Copy link
Member

jrafanie commented Sep 1, 2017

Yeah, bin/update in each repo could detect if you're out of date and warn you. It makes sense, bin/update shouldn't do an explicit git pull but could yell at you that you're updating dependencies but that you're x commits behind the remote master branch.

@chrisarcand
Copy link
Member

What @jrafanie said.

@Fryguy
Copy link
Member

Fryguy commented Sep 1, 2017

Trying to understand the core issue, it sounds like you're saying that you have "oldish" code in the main repo, but you happen to pull in "new" code from the other repos? If so, yeah, that's one of the downsides to splitting things up. I agree with @bdunne that a comment in the gitter channel would just be noise that's ignored...for example, does anyone even notice the activity bar on the right side anymore?

I am for @jrafanie's idea in a way... Make ManageIQ/manageiq's bin/update print out a nice red warning (blinking? 🤔) that your branch is not up to date with master and recommend that you rebase against it. From the other perspective, in the bin/update files of the plugin repos, where it sees you are using a symlinked spec/manageiq directory, that can print out a nice red warning that the spec/manageiw is not up to date with master.

@Fryguy
Copy link
Member

Fryguy commented Sep 1, 2017

@NickLaMuro I'm going to close this issue and reopen it on manageiq itself, since it's not bot related (since it's morphed into not using the bot for notifications).

@Fryguy
Copy link
Member

Fryguy commented Sep 1, 2017

Opened ManageIQ/manageiq#15926

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

No branches or pull requests

5 participants