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

Fix Pipeline overwriting name #4059

Merged
merged 4 commits into from
Jun 21, 2024
Merged

Fix Pipeline overwriting name #4059

merged 4 commits into from
Jun 21, 2024

Conversation

hardillb
Copy link
Contributor

fixes #4058

Description

Looks like the logic got flipped during the changes to #4016

Related Issue(s)

#4058

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

fixes #4058 

Looks like the logic got flipped at some point.
@hardillb hardillb added this to the 2.6 milestone Jun 21, 2024
@hardillb hardillb requested a review from knolleary June 21, 2024 12:26
@hardillb hardillb self-assigned this Jun 21, 2024
@knolleary
Copy link
Member

I disagree. The logic should be: if !merge then overwrite everything. If merge, we skip if on the skip list.

Is the problem a layer up where this function is being called by the pipeline with a false rather than a true` for the merge flag?

We need a unit test to verify the behaviour.

@hardillb
Copy link
Contributor Author

OK, I've moved it up and it does what's expected and doesn't fail any of the existing tests.

I'll try and write a suitable testcase

Copy link

codecov bot commented Jun 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.72%. Comparing base (edf11c8) to head (1b2228d).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4059   +/-   ##
=======================================
  Coverage   78.71%   78.72%           
=======================================
  Files         284      284           
  Lines       13008    13008           
  Branches     2897     2897           
=======================================
+ Hits        10239    10240    +1     
+ Misses       2769     2768    -1     
Flag Coverage Δ
backend 78.72% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hardillb
Copy link
Contributor Author

@knolleary ready for another look

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Looks good. Added just the test case to my local and it failed (as expected). Then applied the fix and it passes (as expected).

@knolleary knolleary merged commit 94e85f9 into main Jun 21, 2024
14 checks passed
@knolleary knolleary deleted the hardillb-patch-1 branch June 21, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instance name is copied over when deploying DevOps Pipelines
2 participants