Skip to content

Add instructions how to work with fetchers #6731

Merged
Siedlerchr merged 4 commits into
masterfrom
add-fetcher-docs
Aug 5, 2020
Merged

Add instructions how to work with fetchers #6731
Siedlerchr merged 4 commits into
masterfrom
add-fetcher-docs

Conversation

@koppor

@koppor koppor commented Aug 2, 2020

Copy link
Copy Markdown
Member

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.

@tobiasdiez tobiasdiez left a comment

Copy link
Copy Markdown
Member

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?

Comment thread docs/advanced-reading/fetchers.md Outdated
```

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
Copy Markdown
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

koppor commented Aug 4, 2020

Copy link
Copy Markdown
Member Author

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

koppor commented Aug 4, 2020

Copy link
Copy Markdown
Member Author

We try to have two different keys:

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

@koppor

koppor commented Aug 4, 2020

Copy link
Copy Markdown
Member Author

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