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

Fixes #6828/BZ1104636: show correct version of puppet module. #4501

Merged
merged 1 commit into from Aug 11, 2014

Conversation

waldenraines
Copy link
Contributor

The latest version of puppet modules was being incorrectly
calculated, based on an incorrect assumption that the last
item in the array was the latest version. This commit
explicitly sorts the puppet modules by the version such that
the latest version is correct.

http://projects.theforeman.org/issues/6828
https://bugzilla.redhat.com/show_bug.cgi?id=1104636

return puppetModule.version;
});

latest = angular.copy(puppetModules[puppetModules.length - 1]);
Copy link
Member

Choose a reason for hiding this comment

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

While I agree this is OK for the client to ensure, what about default ordering of puppet modules coming from the server and/or requesting sorted modules from the server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do that if we really wanted to, however, I went ahead and put it here since we're already modifying the response here. I could move all of this logic to the server though if you have a strong preference.

Copy link
Member

Choose a reason for hiding this comment

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

What is this API returning in terms of data right now? I can see returning the data by default sorted by version as helpful to more than just the UI. Or at least, accepting sort params and us calling the API to get the data sorted how we want as I think this benefits more use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the API simply returns the puppet module versions in an nondeterministic order.

Copy link
Member

Choose a reason for hiding this comment

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

I still think the API is the best place to do it as there is more benefit there and this keeps ordering more consistent with how we do it in other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you're going the API route, you may want to consider removing the available_puppet_modules action and just using puppet_modules#index. I think you could just add some filters like not_in_content_view_id, name, etc. along with your sort param.

@waldenraines
Copy link
Contributor Author

Blocked by #4540.

The latest version of puppet modules was being incorrectly
calculated, based on an incorrect assumption that the last
item in the array was the latest version.  This commit
explicitly sorts the puppet modules by the version such that
the latest version is correct.

http://projects.theforeman.org/issues/6828
https://bugzilla.redhat.com/show_bug.cgi?id=1104636
@waldenraines
Copy link
Contributor Author

@ehelms updated, I could not do the grouping server side (and thus the "Use Latest") because we don't support that in items.rb but I did move the sorting to the server.

@bbuckingham
Copy link
Member

ACK

waldenraines pushed a commit that referenced this pull request Aug 11, 2014
Fixes #6828/BZ1104636: show correct version of puppet module.
@waldenraines waldenraines merged commit 14befa0 into Katello:master Aug 11, 2014
@waldenraines waldenraines deleted the 6828 branch August 11, 2014 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants