parallels: simplify regex for vm files #108
Conversation
| @@ -41,7 +41,7 @@ func (p *ParallelsProvider) Process(ui packersdk.Ui, artifact packersdk.Artifact | |||
| } | |||
|
|
|||
| tmpPath := filepath.ToSlash(path) | |||
| pathRe := regexp.MustCompile(`^(.+?)([^/]+\.(pvm|macvm)/.+?)$`) | |||
| pathRe := regexp.MustCompile(`[^/]+\.(pvm|macvm)/.+$`) | |||
There was a problem hiding this comment.
is pvmPath = filepath.FromSlash(matches[2]) still correct since there is only one capture would this be at index 1?
I think it would be great to add a test for this. It wasn't clear to me what the actual file paths can be.
There was a problem hiding this comment.
That is a good question, I tested this with the updated test that I added to #107, and it did not fail fwiw, but I'll check what happens here. I would think there's two matches only indeed, the global one and the pvm/macvm.
I'll investigate, thanks for the callout
There was a problem hiding this comment.
Alright nevermind, I guess I didn't do my test right and it does crash now, I'll fix that now
There was a problem hiding this comment.
Rerolled, since we don't capture the whole path now, but instead only the .+.pvm|macvm/..., we don't need to get a submatch and instead we can use FindString to return the whole path, excluding the parent directories.
There was a problem hiding this comment.
Note: I based this PR on #107 for tests to run, we can rebase it on main before merging
There was a problem hiding this comment.
Thanks for making the update. I rebased onto the latest main and will merge once all goes green.
lbajolet-hashicorp
force-pushed
the
parallels_regex_cleanup
branch
2 times, most recently
from
10ed84f to
c5a2cf4
Compare
The original regex had multiple occurrences of a `.+?' pattern, which is essentially the same as a `.*', so we replace the former by the latter for clarity. Additionally, since the beginning of the path does not interest us, we can ignore it from the regex, and only start matching once we have found what's interesting for us, further simplifying the expression.
nywilken
force-pushed
the
parallels_regex_cleanup
branch
from
c5a2cf4 to
8f85b4b
Compare
| @@ -41,7 +41,7 @@ func (p *ParallelsProvider) Process(ui packersdk.Ui, artifact packersdk.Artifact | |||
| } | |||
|
|
|||
| tmpPath := filepath.ToSlash(path) | |||
| pathRe := regexp.MustCompile(`^(.+?)([^/]+\.(pvm|macvm)/.+?)$`) | |||
| pathRe := regexp.MustCompile(`[^/]+\.(pvm|macvm)/.+$`) | |||
There was a problem hiding this comment.
Thanks for making the update. I rebased onto the latest main and will merge once all goes green.
The original regex had multiple occurrences of a
.+?pattern, which is essentially the same as a.*, so we replace the former by the latter for clarity.Additionally, since the beginning of the path does not interest us, we can ignore it from the regex, and only start matching once we have found what's interesting for us, further simplifying the expression.