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

Rename "filename" to "name" in ActiveKernelModule model (closes issue #47) #169

Closed
wants to merge 3 commits into from

Conversation

seeni-dev
Copy link

renamed filename to name in ActiveKernelModule implementation and usage of that class

Copy link
Member

@examon examon left a comment

Choose a reason for hiding this comment

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

When you change filename to name, make sure that you do not break any other existing actors.

Your change breaks checkbtrfs actor, because it still uses filename.
https://github.com/oamg/leapp-repository/blob/master/repos/system_upgrade/el7toel8/actors/checkbtrfs/actor.py
https://github.com/oamg/leapp-repository/blob/master/repos/system_upgrade/el7toel8/actors/checkbtrfs/tests/test_btrfs.py

Please make changes to checkbtrfs actors as well.

@examon examon changed the title closes issue #47 Rename "filename" to "name" in ActiveKernelModule model (closes issue #47) Apr 23, 2019
Copy link
Member

@pirat89 pirat89 left a comment

Choose a reason for hiding this comment

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

Still broken. There are still actors that use filename instead of name. Like *sctp*. Please, verify all actors and try to run the upgrade to test the functionality.

@pirat89
Copy link
Member

pirat89 commented Apr 26, 2019

Additionally, fix commit message: https://chris.beams.io/posts/git-commit/

@examon
Copy link
Member

examon commented Jun 7, 2019

@pirat89 Close this one? It looks like it is not moving forward.

@seeni-dev
Copy link
Author

No no. I will work on this.

@pirat89
Copy link
Member

pirat89 commented Jun 11, 2019

@seenivasanseeni regarding the changes we made in the meantime, rebase the branch against upstream master please. You will have better clue about results of the tests then.

@leapp-bot
Copy link
Collaborator

Please make sure, that you use request review each time you make changes in your code, so the reviewer will be notified about your request

@leapp-bot
Copy link
Collaborator

This PR has been linked in issue tracker (#RHELLEAPP-1736)

@bocekm
Copy link
Member

bocekm commented Aug 9, 2019

@seenivasanseeni, do you plan to do the requested changes?

@seeni-dev
Copy link
Author

Since the branch has moved further, I will start fresh from the new master. I am gonna close this PR.

@seeni-dev seeni-dev closed this Aug 9, 2019
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

Successfully merging this pull request may close these issues.

5 participants