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

Feature/allow single submissions and path names #1935

Conversation

euberdeveloper
Copy link

@euberdeveloper euberdeveloper commented Aug 26, 2024

This pull request add minor updates described in the last comment of #1852. It fixed #1848 and #1849

Copy link
Member

@Kr0nox Kr0nox left a comment

Choose a reason for hiding this comment

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

As mentioned on the last PR, since you are changing/adding features on how submissions can be given to Jplag, please add an e2e case for each of them.

Here are references on where they are implemented currently:
develop/report-viewer/tests/e2e/OpenComparisonTest.spec.ts
develop/.github/workflows/complete-e2e.yml

Please add one for every Problem solved by this PR

@euberdeveloper
Copy link
Author

Ok, the tests I did so far weren't in the e2e, I'll add some

@euberdeveloper
Copy link
Author

The functioning of the e2e tests seems to be more complicated than the normal Java tests.
If I've understook correctly, for each featur I should:

  1. Add a zip dataset to .github/workflows/files
  2. During the action, java -jar jplag.jar ${{ matrix.dataset.folder }} -l ${{ matrix.dataset.language }} -r ${{ matrix.dataset.name }}-report ${{ matrix.dataset.cliArgs }} will execute it
  3. The report result will be added to an artifact
  4. I should add here an object with the expected first two submission names that will be compared
  5. The e2e tests of Vue will do the rest

Is it correct?

@euberdeveloper

This comment was marked as outdated.

@euberdeveloper

This comment was marked as outdated.

@euberdeveloper euberdeveloper requested a review from Kr0nox August 27, 2024 15:27
@euberdeveloper

This comment was marked as outdated.

@euberdeveloper

This comment was marked as outdated.

@tsaglam tsaglam self-requested a review August 29, 2024 13:50
@tsaglam tsaglam added enhancement Issue/PR that involves features, improvements and other changes major Major issue/feature/contribution/change labels Aug 30, 2024
Copy link

sonarqubecloud bot commented Sep 5, 2024

@euberdeveloper
Copy link
Author

euberdeveloper commented Sep 5, 2024

In this check, the error is Set the SONAR_TOKEN env variable.. Is it something that should be done on your side? @Kr0nox @tsaglam

Otherwise all checks seem to have passed!

Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

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

I finally found some time to take a look at the PR. Understanding what was part of this PR and what was not was a bit tricky due to the lack of a PR description.

If I understand correctly, this PR currently contains two changes:

  • Fix an issue where single-submission root directory + single-submission old directory was not accepted
  • Fix the issue with multiple root folders that have the same name by using the shortest uncommon path

We discussed these features internally. The first one is excellent; we would like to include it. We currently do not want to support the second one, as including super directories in the root names exposes them in the report zip. This means a user could expose information about their system via a report that might not be intended.

Sorry for giving you this feedback so late.

@euberdeveloper
Copy link
Author

Ok, so should I divide the pull request in two pull requests?

@euberdeveloper
Copy link
Author

Also for the second feature, we could make it that it stops until the current dir where it is executed JPlag CLI or something similar so that it doesn't give further than needed

@tsaglam
Copy link
Member

tsaglam commented Sep 6, 2024

Ok, so should I divide the pull request in two pull requests?

Yes, the first feature should be its own PR.

Also for the second feature, we could make it that it stops until the current dir where it is executed JPlag CLI or something similar so that it doesn't give further than needed

I will take that idea into our internal discussions.

@euberdeveloper
Copy link
Author

Ok, then:

  1. I will close this PR and make a new PR only with the accepted feature
  2. I will open another PR with the unaccepted one, in which in case some changes could be proposed
    • e.g. stopping at cwd or something similar when proceeding to the parents
    • e.g. adding a flag "unsafe" or "reveal paths" and only if active the feature will act

I think with a specific flag to enable the feature the security issue is solved, because one that wants to use it must explicitly specify that the behaviour is wanted

@tsaglam
Copy link
Member

tsaglam commented Sep 13, 2024

Closed, as discussed above.

@tsaglam tsaglam closed this Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue/PR that involves features, improvements and other changes major Major issue/feature/contribution/change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants