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

Moving common function from DistGitAPI to GitOperations and Dockerfile #73

Merged
merged 16 commits into from
Mar 20, 2023
Merged

Moving common function from DistGitAPI to GitOperations and Dockerfile #73

merged 16 commits into from
Mar 20, 2023

Conversation

phracek
Copy link
Member

@phracek phracek commented Feb 27, 2023

What is done in this function:

  • Dockerfile.generator is moved from F34 -> F36
  • Dockerfile.tests is moved from F34->F36
  • Functions get_from_df, get_from, set_from, update_dockerfile, and update_dockerfile_rebuild are moved from distgit.py to dockerfile.py
  • Functions set_commit_msg, _do_git_reset, _clone_upstream, _get_unpushed_commits, show_git_changes, update_variable_in_string, update_test_openshift_yaml, get_commit_msg, _pull_upstream are moved from distgit.py to git_operations.py
  • Functions copy_upstream2downstream, handle_dangling_symlinks are moved from distgit.py to sync.py file.

All there changes are check by Test Suite.

@phracek
Moving common function from DistGitAPI to GitOperations and Dockerfile
e8f46f7 
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
@phracek
Copy link
Member Author

phracek commented Feb 27, 2023

/test

frenzymadness
frenzymadness reviewed Mar 2, 2023
View reviewed changes
Copy link
Member

@frenzymadness frenzymadness left a comment

Choose a reason for hiding this comment

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

Let me know if anything here needs more context or if you want to have a meeting and discuss my comments here. A lot of them are just opinions.

container_workflow_tool/dockerfile.py Outdated Show resolved Hide resolved
container_workflow_tool/dockerfile.py Outdated Show resolved Hide resolved
container_workflow_tool/dockerfile.py Outdated Show resolved Hide resolved
container_workflow_tool/dockerfile.py Outdated Show resolved Hide resolved
container_workflow_tool/dockerfile.py Outdated Show resolved Hide resolved
container_workflow_tool/sync.py Outdated Show resolved Hide resolved
tests/test_git_operations.py Show resolved Hide resolved
tests/test_git_operations.py Outdated Show resolved Hide resolved
tests/test_git_operations.py Outdated Show resolved Hide resolved
tests/test_git_operations.py Outdated Show resolved Hide resolved
phracek and others added 8 commits March 9, 2023 09:10
@phracek @frenzymadness
Update container_workflow_tool/dockerfile.py
2ed96b7 
Co-authored-by: Lumír 'Frenzy' Balhar <frenzy.madness@gmail.com>
@phracek @frenzymadness
Update container_workflow_tool/dockerfile.py
71a32ca 
Co-authored-by: Lumír 'Frenzy' Balhar <frenzy.madness@gmail.com>
@phracek @frenzymadness
Update comment
5d4c85c 
Co-authored-by: Lumír 'Frenzy' Balhar <frenzy.madness@gmail.com>
@phracek @frenzymadness
Update container_workflow_tool/git_operations.py
df2f575 
Co-authored-by: Lumír 'Frenzy' Balhar <frenzy.madness@gmail.com>
@phracek @frenzymadness
Update container_workflow_tool/git_operations.py
65b93ca 
Co-authored-by: Lumír 'Frenzy' Balhar <frenzy.madness@gmail.com>
@phracek @frenzymadness
Update container_workflow_tool/git_operations.py
48ccdfb 
Co-authored-by: Lumír 'Frenzy' Balhar <frenzy.madness@gmail.com>
@phracek @frenzymadness
Update tests/test_git_operations.py
d9adbe5 
Co-authored-by: Lumír 'Frenzy' Balhar <frenzy.madness@gmail.com>
@phracek
Fix issues caught by review
6031d6e 
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
@phracek
Call commands if iterable
d6f76bf 
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
@phracek @pkubatrh
Update container_workflow_tool/git_operations.py
c562a02 
Co-authored-by: Petr Kubat <pkubat@redhat.com>
@phracek
Return back git config core.pager
1783748 
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
frenzymadness
frenzymadness reviewed Mar 14, 2023
View reviewed changes
Copy link
Member

@frenzymadness frenzymadness left a comment

Choose a reason for hiding this comment

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

Last two comments. I understand that the code is not all yours. If you'd like me to take a closer look at the code as a whole, feel free to ping me.

container_workflow_tool/dockerfile.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
phracek and others added 3 commits March 15, 2023 12:37
@phracek
Use only update_dockerfile_rebuild
4bda3ce 
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
@phracek @frenzymadness
Fix test_wrong_usage
ee94d3b 
Co-authored-by: Lumír 'Frenzy' Balhar <frenzy.madness@gmail.com>
@phracek
Use only update_dockerfile_rebuild
233313d 
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
@phracek
Copy link
Member Author

phracek commented Mar 15, 2023

/test

@phracek
Copy link
Member Author

phracek commented Mar 15, 2023

@frenzymadness Your comments are great. For now, that is enough. Last two comments, were addressed, including adding more tests.

frenzymadness
frenzymadness reviewed Mar 16, 2023
View reviewed changes
Copy link
Member

@frenzymadness frenzymadness left a comment

Choose a reason for hiding this comment

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

Last couple of nitpicks.

container_workflow_tool/dockerfile.py Outdated Show resolved Hide resolved
container_workflow_tool/dockerfile.py Show resolved Hide resolved
tests/test_dockerfile.py Outdated Show resolved Hide resolved
tests/test_git_operations.py Show resolved Hide resolved
@phracek
Fix issues by review. Proper get_from handling
43ecfda 
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
frenzymadness
frenzymadness approved these changes Mar 20, 2023
View reviewed changes
Copy link
Member

@frenzymadness frenzymadness left a comment

Choose a reason for hiding this comment

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

I have nothing more to add. Good job!

@phracek phracek merged commit a716dda into master Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkubatrh pkubatrh pkubatrh left review comments

@frenzymadness frenzymadness frenzymadness 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.
3 participants
@phracek @pkubatrh @frenzymadness