Skip to content

Implement a fix to the issue raised in #228 #229

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
Closed
wants to merge 5 commits into from
Closed

Implement a fix to the issue raised in #228 #229

wants to merge 5 commits into from

Conversation

rasmunk
Copy link
Contributor

@rasmunk rasmunk commented Apr 16, 2025

Aims to fix the issue raised in #228

@rasmunk rasmunk requested review from a team, Martin-Rehr and jonasbardino and removed request for a team April 16, 2025 12:55
jonasbardino
jonasbardino approved these changes Apr 23, 2025
View reviewed changes
Copy link
Contributor

@jonasbardino jonasbardino left a comment

Choose a reason for hiding this comment

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

Yes, either the conditionals need to use e.g. a plain boolean check to err out if transfer_dict is an empty dict, or the transfer_map.get() should default to None and transfer_dict be initialized as empty dict in the 'create' part of the else case .

@jonasbardino jonasbardino self-assigned this Apr 23, 2025
@jonasbardino jonasbardino added the bug Something isn't working label Apr 23, 2025
@jonasbardino jonasbardino linked an issue Apr 23, 2025 that may be closed by this pull request
The conditional check in datatransfer does not validate whether the required transfer_id is set #228
Open
@albu-diku
Copy link

Perhaps this would benefit from having a test?

@rasmunk
Copy link
Contributor Author

rasmunk commented Apr 25, 2025 •
edited
Loading

Perhaps this would benefit from having a test?

That is a good shout, I guess the best place for it would be the existing tests/test_mig_shared_transferfunctions.py right? Or maybe a brand new one is required with inspiration from tests/test_mig_shared_functionality_cat.py

@jonasbardino
Copy link
Contributor

Not sure how easy it is to in fact test this case in a meaningful way, but feel free to use either that existing test suite for mig/shared/transferfunctions.py in tests/test_mig_shared_transferfunctions.py or add a new tests/test_mig_shared_functionality_datatransfer.py for the purpose.

@rasmunk
Add basic testing for validating that requesting a deltransfer or red…
6c14328 
…otransfer without the transfer_id against the datatransfer functionality throws the expected error to the user. Also makes tchanges to the datatransfer.py module to allow for this testing and linting recommendations
@rasmunk
Copy link
Contributor Author

rasmunk commented Apr 28, 2025 •
edited
Loading

The latest commit makes additional changes that enables basic testing against the datatransfer functionality with the tests/test_mig_shared_functionality_datatransfer.py module. This however makes this pull request leave the easy to merge track due to the changes necessary in the tree to implement the tests. I will report back once these changes have been tested on an actual system deployment

jonasbardino
jonasbardino approved these changes May 7, 2025
View reviewed changes
Copy link
Contributor

@jonasbardino jonasbardino left a comment

Choose a reason for hiding this comment

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

Approved. Looks good, and just a pedantic formal comment :)
I think we want at least the actual fix in shared/functionality/datatransfer.py merged through svn or backported there and in edge/main.

jonasbardino pushed a commit that referenced this pull request May 9, 2025
@rasmunk
Add the changes specified in the #229 pull request on migrid-sync tha…
436347f 
…t amends and fixes an issue in the datatransfer module

git-svn-id: svn+ssh://svn.code.sf.net/p/migrid/code/trunk@6229 b75ad72c-e7d7-11dd-a971-7dbc132099af
rasmunk added a commit that referenced this pull request May 9, 2025
@rasmunk
Add the changes specified in the #229 pull request on migrid-sync tha…
cfa02fe 
…t amends and fixes an issue in the datatransfer module

git-svn-id: svn+ssh://svn.code.sf.net/p/migrid/code/trunk@6229 b75ad72c-e7d7-11dd-a971-7dbc132099af
@rasmunk
Copy link
Contributor Author

rasmunk commented May 9, 2025

Manually merged via svn and local git operations

@rasmunk rasmunk closed this May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonasbardino jonasbardino jonasbardino approved these changes

@Martin-Rehr Martin-Rehr Awaiting requested review from Martin-Rehr

Assignees

@jonasbardino jonasbardino

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The conditional check in datatransfer does not validate whether the required transfer_id is set
3 participants
@rasmunk @albu-diku @jonasbardino