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

Add instructions how to work with fetchers #6731

Merged
merged 4 commits into from
Aug 5, 2020
Merged

Conversation

koppor
Copy link
Member

@koppor koppor commented Aug 2, 2020

This PR tries to summarize of how to work with fetchers locally.

I also added a fallback to environemnt variables in BuildInfo.java as I could not get it running locally.

My change in BuildInfo.java makes the handling more newcomer-friendly.

I am also opne to revert that change and to really desribe the IDE setup in the case of IntelliJ (and Eclipse) to enfore the reading of the "correct" build.properties even when running from the IDE (and not from gradle)

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Although environment variables are an improvement over the current situation, it would still restrict the people that can run the fetcher tests to the core team (since no one else has access to them). What about creating new dedicated keys for local development, which can be in the source code directly as fall-back options?

```

When executing `./gradlewrun`, gradle executes `processResources` and populates `build.properties` accordingly.
However, when working directly in the IDE, IntelliJ keeps reading `build.properties` from `src/main/resources`.
Copy link
Member

Choose a reason for hiding this comment

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

You can also run the processResources before a normal run with intellj (automatically in the run config). That works like a charm for me.

@koppor
Copy link
Member Author

koppor commented Aug 4, 2020

Similar issue for Eclipse or other IDEs. - When the keys are included in the source, we need to renew them to avoid hitting the API limit (you know, information leakage, ...)

With the the proposed PR, we are still in the situation, that only the core team can currently execute the fetcher tests. Alternatively the ones who are smart enough to read our build.gradle file and be able to use file search.

@koppor
Copy link
Member Author

koppor commented Aug 4, 2020

We try to have two different keys:

  • production key (distributed)
  • CI/CD key together with test key (included)

@koppor
Copy link
Member Author

koppor commented Aug 4, 2020

Requested changes implemented 😅

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 4, 2020
@Siedlerchr Siedlerchr merged commit 3d68514 into master Aug 5, 2020
@Siedlerchr Siedlerchr deleted the add-fetcher-docs branch August 5, 2020 13:09
Siedlerchr added a commit that referenced this pull request Aug 9, 2020
* upstream/master: (47 commits)
  Fix copy pasting and delete via menu or key (#6740)
  Add instructions how to work with fetchers  (#6731)
  Autoinstall extension in chrome (#6442)
  Delete link after download (#6723)
  New translations JabRef_en.properties (Portuguese, Brazilian) (#6728)
  Bump pascalgn/automerge-action from v0.8.5 to v0.9.0 (#6736)
  Bump byte-buddy-parent from 1.10.13 to 1.10.14 (#6733)
  Bump mockito-core from 3.4.4 to 3.4.6 (#6734)
  Bump unirest-java from 3.8.06 to 3.9.00 (#6735)
  Bump org.beryx.jlink from 2.21.1 to 2.21.2 (#6732)
  Add testing interface, including a set of capabilities to tests for (#6687)
  Fix pasting on mac and linux (#6419)
  Add validation of "AUTHORS" file (#6722)
  Squashed 'src/main/resources/csl-styles/' changes from cacc4ee..827b986
  New Crowdin updates (#6721)
  Add missing AUTHORs
  Fix for issue 6639 (#6719)
  Fix more links
  Fix link
  New Crowdin updates (#6718)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants