search

exercism / v2-configlet

Tool to assist in managing Exercism language tracks.
MIT License
16 stars 23 forks source link

lint: duplicateTrackUUID check fails if the repository name is not correct #80

Open icyrockcom opened 7 years ago

icyrockcom commented 7 years ago

I have a fork of the purescript track, but I still have the old repository name:

https://github.com/icyrockcom/xpurescript

Looks like the track ID is determined based on the repository folder name:

https://github.com/exercism/configlet/blob/master/track/track.go#L27

In a sample build in my personal travis build setup:

https://travis-ci.org/icyrockcom/xpurescript/builds/278841675

the linter fails with:

...
$ bin/configlet lint .
-> The following UUID was found in multiple Exercism tracks. Each exercise UUID must be unique across tracks.
c41a2961-f7ca-4eaf-83cf-48762fc39c06
...

since it thinks the track is called xpurescript, not purescript.

A few ways to resolve this with the current setup come to mind:

In order to avoid falling back to these workarounds, it might be better to store the track ID in config.json and read it from there, with a fall-back to the folder name.

kytrinyx commented 7 years ago

This is a good point, thank you.

Rename the repository, which in my case is fine, but other people may not be able to do that.

Yeah, we've run into that a couple of times with language contributors who also contribute to Exercism tracks.

In order to avoid falling back to these workarounds, it might be better to store the track ID in config.json and read it from there, with a fall-back to the folder name.

I removed the track ID from the config.json since it wasn't being used anywhere on the site or by the tools, but this is a valid use case, and I think that having the track ID in the config is the cleanest solution.

I can script this up to submit the change to all the tracks.

icyrockcom commented 7 years ago

Ah, I did not know it was there in the first place :) OK, cool, it'll be easy for me to test it out afterwards.

kytrinyx commented 7 years ago

@robphoenix @nywilken there are a couple of ways of doing this. We could either provide the track ID in all the config.json files, or we could update the commands to take a --track-id flag (optional, default to the directory name), and update the tracks to use that.

Thoughts? Preferences?

robphoenix commented 7 years ago

I've run into this myself, having the Go track forked to robphoenix/exercism-go.

I think I'd prefer to see the track id in the config.json files, I'd rather not expect users to have to type configlet lint . --track-id=go. I realise this could be more effort as it means updating all track config,json files. It could be beneficial to add the --track-id flag anyway as well, this would be an easy quick fix for now at the very least.

Insti commented 7 years ago

I'd prefer to see the track id in the config.json files

👍

rpottsoh commented 7 years ago

@robphoenix said:

I've run into this myself, having the Go track forked to robphoenix/exercism-go.

My fork for Delphi is named similarly as you did for Go.

I have been dealing with this issue, in silence. Finally got fed up and came looking around here to see if anyone else was experiencing this. Glad to see that a fix is on the horizon. :tada:

Questions:

  1. Is the error message reported by configlet, regarding the UUID(s), accurate in this circumstance?
  2. If configlet can't determine if the UUID(s) are unique because it can't actually compare them should a different error message be presented?
nywilken commented 7 years ago

@robphoenix @nywilken there are a couple of ways of doing this. We could either provide the track ID in all the config.json files, or we could update the commands to take a --track-id flag (optional, default to the directory name), and update the tracks to use that.

I think having track ID in the config.json would be the ideal solution. As @robphoenix pointed requiring the user to set the track at run time would be a bit much to ask. Especially, for new contributors using configlet for the first time. The UUID validation service will display errors if you give the wrong track id, which may lead to more confusion.

With that said, @robphoenix I see you implemented --track-id as a flag. If it is just for easy of roll out, I would suggest that we go the config.json route and default to the directory name for track-id if the config.json does not have the property. This would allow us to get a fix out and let maintainers update the config files when they have a moment. I think 🤔

robphoenix commented 7 years ago

This would allow us to get a fix out and let maintainers update the config files when they have a moment.

Yep, this is what I was thinking with #90

kytrinyx commented 7 years ago

If it is just for easy of roll out, I would suggest that we go the config.json route and default to the directory name for track-id if the config.json does not have the property.

Agreed. Suspenders and a belt. Let's do it.

nywilken commented 7 years ago

@icyrockcom the latest release of configlet has support for a new --track-id flag that can be passed to the lint command, which should remedy this issue for now. There are plans to add support for a track_id within a track's config.json in the future.

rpottsoh commented 7 years ago

@nywilken Antivirus is complaining that there is a virus detected within the 32 bit windows release. Trojan:Win32/Fuerboos.A!cl is what is being reported.

nywilken commented 7 years ago

Hmmm, not able to find too much information here https://www.microsoft.com/en-us/wdsi/threats/malware-encyclopedia-description?Name=Trojan:Win32/Fuerboos.A!cl. Looks like it is some cloud based heuristic for catching potential malware. I will regenerate and test on my Windows machine. After I re-release would you mind testing again and maybe submitting the file to https://www.microsoft.com/en-us/wdsi/filesubmission

rpottsoh commented 7 years ago

No problem. Just leave a note here to let me know it is ready to try again.

nywilken commented 7 years ago

New versions of the Win32 and Win64 binaries have been released. I'm testing on Windows 8 with Windows Defender - no issues so far. If you are still seeing the alert lets open an issue and we can track it there. Thanks so much for testing and your help.

rpottsoh commented 7 years ago

I am using MSE and I think it is being stupid or at least overly cautious. I turned off real-time protection and the download completed fine. I then had MSE scan the file and it found nothing. I then extracted the EXE and had MSE scan that, it found nothing. I will pass the ZIP file on to the submission link you provided and indicate that the file is being incorrectly flagged as being infected.....

Thanks for the new release, I am excited to check out the --track-id option and the new tree command.

rpottsoh commented 7 years ago

Good news and bad news: Good news is is that --track-id and tree work great! Thanks

Bad news is that I have to turn off MSE's real-time protection to allow configlet to run. I have submitted the ZIP file to Microsoft.... Who knows what happens next, or when.

nywilken commented 7 years ago

Thanks for submitting the file for analysis. It seems like a number of applications have been hit with this alert - looks like the tool might still be in its early stages. Various sources stated that after submitting a legit file for analysis a new set of definitions were made available, which no longer flagged the binaries under test. I hope its a quick turn around on the MSE side. In the mean, I will keep an eye out for this issue and check if it is related to Go cross compilation in anyway.

rpottsoh commented 7 years ago

The file I submitted for checking earlier today received a clean bill of health. So, for now, I have instructed MSE to exclude configlet.exe from its gaze.

icyrockcom commented 7 years ago

@nywilken Thanks! Confirmed to be working now on purescript track: https://travis-ci.org/icyrockcom/xpurescript/builds/290680140

iHiD commented 5 years ago

Can this be closed?