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

project_loader: replace dict keys as well as values #2166

Merged

Conversation

kyrofa
Copy link
Contributor

@kyrofa kyrofa commented Jun 22, 2018

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

The snapcraft CLI actually runs a text replacement over the snapcraft.yaml, replacing all occurrences of its built-in variables (such as $SNAPCRAFT_STAGE) with their actual values, knowing that, once these are migrated into the final snap.yaml the variables have no meaning. However, when it comes to running the replacements on dicts, it only checks the values for replacement, not the keys. This PR fixes LP: #1746636 by simply running replacements on the keys as well as the values.

@kubiko
Copy link
Contributor

kubiko commented Jun 22, 2018

I can confirm this is fixing bug LP: #1778287

@sergiusens
Copy link
Collaborator

This should also fix the organize bug, https://bugs.launchpad.net/snapcraft/+bug/1746636, so I am marking LP: #1778287 as a dup of that one.

Copy link
Collaborator

@sergiusens sergiusens 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, only reason I am marking as needs fixing is that it seems this was a random request for which we already had a bug, so after affecting the original bug we should be ok.

The snapcraft CLI actually runs a text replacement over the
`snapcraft.yaml`, replacing all occurrences of its built-in variables
(such as `$SNAPCRAFT_STAGE`) with their actual values, knowing that,
once these are migrated into the final `snap.yaml` the variables have no
meaning. However, when it comes to running the replacements on dicts, it
only checks the values for replacement, not the keys. Fix this by simply
running replacements on the keys as well as the values.

LP: #1746636

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
@kyrofa kyrofa force-pushed the bugfix/1778287/incomplete_replacements branch from 3631a85 to 0d94216 Compare June 25, 2018 16:39
@codecov-io
Copy link

codecov-io commented Jun 25, 2018

Codecov Report

Merging #2166 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2166      +/-   ##
==========================================
+ Coverage   91.29%   91.29%   +<.01%     
==========================================
  Files         200      200              
  Lines       12580    12585       +5     
  Branches     1871     1871              
==========================================
+ Hits        11485    11490       +5     
  Misses        741      741              
  Partials      354      354
Impacted Files Coverage Δ
snapcraft/internal/project_loader/__init__.py 92% <100%> (+2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f80a10...7e9d2c5. Read the comment docs.

@kyrofa
Copy link
Contributor Author

kyrofa commented Jun 25, 2018

[...] for which we already had a bug

Good catch @sergiusens, forgot about that one. I've fixed the commit as well as the PR description.

@lucyllewy
Copy link
Contributor

Two potentially related bugs that would be good to get squished at the same time: LP#1746633 and LP#1766878

@kyrofa kyrofa merged commit 95c8428 into canonical:master Jun 26, 2018
@kyrofa kyrofa deleted the bugfix/1778287/incomplete_replacements branch June 26, 2018 14:30
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.

None yet

5 participants