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.

Sign up for GitHub

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

Jump to bottom

Bugfix/backend file #5183

Merged
merged 3 commits into from
Jun 26, 2018
Merged

Bugfix/backend file #5183

merged 3 commits into from
Jun 26, 2018

Conversation

bgeuken
Copy link
Member

@bgeuken bgeuken commented Jun 21, 2018

No description provided.

@bgeuken bgeuken mentioned this pull request Jun 21, 2018
Closed
@bgeuken bgeuken requested a review from DavidKang June 21, 2018 11:59
coolo
coolo reviewed Jun 21, 2018
View reviewed changes
src/api/app/models/package.rb Outdated
@@ -1457,7 +1457,7 @@ def last_build_reason(repo, arch, package_name = nil)
arch: arch
)

data = Xmlhash.parse(xml_data.to_s)
data = Xmlhash.parse(xml_data.to_s.to_s)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the ultimate sign that to_s just has the wrong name

Copy link
Member Author

Choose a reason for hiding this comment

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

Definetly:+1:

@bgeuken bgeuken added the DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR label Jun 21, 2018
@bgeuken bgeuken removed the DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR label Jun 21, 2018
Ana06
Ana06 approved these changes Jun 21, 2018
View reviewed changes
src/api/lib/backend/file.rb
@@ -55,8 +55,8 @@ def file(query = {})
nil
end

# Converts file into a String if it's valid
def to_s(query = {})
# Converts file into a String if it's valid and nil if it's not
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment really needed? The method is pretty clear... 🤔

src/api/lib/backend/file.rb Outdated
# Converts file into a String if it's valid
def to_s(query = {})
# Converts file into a String if it's valid and nil if it's not
def content(query = {})
file(query)
@file && valid? ? ::File.open(@file.path).read : nil
Copy link
Member

Choose a reason for hiding this comment

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

There should be a Rubocop cop to avoid using nil like this... :sob

@bgeuken bgeuken force-pushed the bugfix/backend_file_ branch 3 times, most recently from d75c573 to a35cca0 Compare June 22, 2018 06:35
@coolo coolo added the DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR label Jun 22, 2018
@coolo
Copy link
Member

coolo commented Jun 22, 2018

Please don't merge this until #5140 is in - or you break the existing engine (as it's using .to_s)

@coolo coolo removed the DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR label Jun 22, 2018
@bgeuken
[frontend] Drop BuildReasonFile model
2b108f0 
The BuildReasonFile model was abstracting the _reason file that is
stored in the backend.
Though the only place where this method was used we parsed the raw xml
and created another object (PackageBuildReason) out of it.

Because of that it makes sense to drop this model and use a Backend::Api
call like we already do elsewhere.

In addition this solves an issue caused by BuildReasonFile#to_s returning
nil when the file isn't available or invalid.

Kudos for the idea goes to @DavidKang
@bgeuken
[frontend] Refactor BackendFile#to_s method
aeac96d
@bgeuken bgeuken force-pushed the bugfix/backend_file_ branch 2 times, most recently from 6fdd78d to 8c02dd7 Compare June 25, 2018 17:11
@bgeuken
[frontend] Rename BackendFile method from to_s to content
62daff3 
Calling it to_s was a bit misleading since this method would return nil
when the requested file does not exist or is invalid.
@bgeuken
Copy link
Member Author

bgeuken commented Jun 26, 2018

@DavidKang @coolo I had to update a couple of places that were using the backend file class. Would you mind to have a second look?

coolo
coolo approved these changes Jun 26, 2018
View reviewed changes
src/api/app/models/project.rb
@@ -145,14 +145,14 @@ def self.restore(project_name, backend_opts = {})
project = Project.new(name: project_name)

Project.transaction do
project.update_from_xml!(Xmlhash.parse(project.meta.to_s))
project.update_from_xml!(Xmlhash.parse(project.meta.content))
Copy link
Member

Choose a reason for hiding this comment

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

is there any value in having meta not returning content right away? (just curious as you it .meta.content appears quite often in your PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

We would have to look closer into this. But you are right it seems that we only use the content right now.
The other thing is that there is a similar method called config and might be some others. I would keep them in sync.

@bgeuken bgeuken merged commit 4eece6a into openSUSE:master Jun 26, 2018
@bgeuken bgeuken deleted the bugfix/backend_file_ branch June 26, 2018 09:28
@coolo
Copy link
Member

coolo commented Jun 27, 2018

One more test suite problem hidden by cassettes:

--- a/src/api/spec/controllers/webui/project_controller_spec.rb
+++ b/src/api/spec/controllers/webui/project_controller_spec.rb
@@ -1028,7 +1028,7 @@ RSpec.describe Webui::ProjectController, vcr: true do
 
     context 'Can not load project config' do
       before do
-        allow_any_instance_of(ProjectConfigFile).to receive(:to_s).and_return(nil)
+        allow_any_instance_of(ProjectConfigFile).to receive(:content).and_return(nil)
         get :prjconf, params: { project: apache_project }
       end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ana06 Ana06 Ana06 approved these changes

@DavidKang DavidKang DavidKang approved these changes

@coolo coolo coolo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bgeuken @coolo @DavidKang @Ana06