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

Feature/multibuild release #4033

Merged
merged 5 commits into from
Oct 19, 2017
Merged

Conversation

ChrisBr
Copy link
Member

@ChrisBr ChrisBr commented Oct 18, 2017

Cherry-picked the commit from #3672 as @adrianschroeter is currently busy with other stuff.

This is currently blocking us from deployment. As this is already monkey patched on build.o.o and I don't want to deploy and applying this huge patch again.

@mdeniz @evanrolfe please have a look.

@ChrisBr ChrisBr force-pushed the feature/multibuild_release branch 2 times, most recently from 8576030 to cc66be4 Compare October 18, 2017 10:50
@codecov
Copy link

codecov bot commented Oct 18, 2017

Codecov Report

Merging #4033 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4033      +/-   ##
==========================================
+ Coverage   89.17%   89.18%   +<.01%     
==========================================
  Files         309      309              
  Lines       18221    18236      +15     
==========================================
+ Hits        16249    16264      +15     
  Misses       1972     1972
Flag Coverage Δ
#api 81.04% <100%> (+0.69%) ⬆️
#rspec 69.3% <36.36%> (-0.49%) ⬇️
#webui 61.65% <29.41%> (-0.04%) ⬇️

@hennevogel hennevogel added the DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR label Oct 18, 2017
Introduced in f08032e.
Quick and dirty c&p from the old test suite to get
the PR merged as it is required by release team.
Introduced in f08032e.
Package.get_by_name_and_project raised an UnknownObject exception for multibuild containers
as the follow_multibuild option was missing. Just using now the pkg object.
@ChrisBr ChrisBr added Feature Monkey Patched 🙈 This is manually applied in production and removed DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR labels Oct 18, 2017
Copy link

@evanrolfe evanrolfe left a comment

Choose a reason for hiding this comment

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

LGTM as a quick and necessary fix for a problem we're currently experiencing.

However I have to say some of the code here is a bit of an eye sore - too much logic in controllers, use of helpers methods to organise business logic, variable names like pkg, parsing of the params with regex in controllers etc.

@ChrisBr
Copy link
Member Author

ChrisBr commented Oct 18, 2017

@evanrolfe Yep! The whole source_controller with more than 1500 + the source_controller_test with more than 2k LOC is a mess 😢 😢

@ChrisBr ChrisBr merged commit a7f8cdb into openSUSE:master Oct 19, 2017
@coolo
Copy link
Member

coolo commented Oct 19, 2017

deploy, deploy!! :)

@ChrisBr
Copy link
Member Author

ChrisBr commented Oct 19, 2017

@coolo already monkeypatched since last week :)

@ChrisBr ChrisBr deleted the feature/multibuild_release branch February 21, 2018 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Monkey Patched 🙈 This is manually applied in production
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants