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

Update yaml files #84

Merged
merged 33 commits into from
Apr 9, 2024
Merged

Update yaml files #84

merged 33 commits into from
Apr 9, 2024

Conversation

FedericoRondoni
Copy link
Contributor

@FedericoRondoni FedericoRondoni commented Jan 25, 2024

CAQ_ports_air_quality E13o_vessel_density_all E13p_vessel_density_cargo E13q_vessel_density_tanker E13r_vessel_density_others C3S_meteorological_variables

Test to understand if the nested DataSource element can be used for the YAML files + integration of the new References element inside the YAML
@FedericoRondoni FedericoRondoni changed the title Update C1_ports_shipping_aq.yaml Update yaml files Feb 16, 2024
@FedericoRondoni
Copy link
Contributor Author

Hi @santilland @lubojr
I don't know if I'm doing something wrong. I tried to modify the YAML files directly from here as we agreed, but I see that "all checks have failed". I don't know what these checks are.
Please tell me if I have to do something different :)

@santilland
Copy link
Collaborator

Hello @FedericoRondoni , we changed the automatic generation and deployment of the catalog for branches, it now possible to specify which collections one wants to test, instead of always generating all collections.
As we need to adapt the catalog generation anyhow once we have some examples the checks are currently not important. For test branches it expects to have information on which collections should be tested, i will change the default behavior for now

@FedericoRondoni
Copy link
Contributor Author

Hi @santilland and thanks for your reply. How can I do that?

@santilland
Copy link
Collaborator

You can write the list of collection identifiers (separated by spaces) you want to test in the body of the pull request. So by editing the first "comment" of the pull request. After you push a commit it should then only generate the collections provided in that list.

@FedericoRondoni
Copy link
Contributor Author

Like this?
Sorry for these questions, but I'm not an expert GitHub user

@santilland
Copy link
Collaborator

No problem, it is not common use what we are doing here, so questions are very much welcome, we will need to better describe and document the process we are still evaluating.
The collection identifier for generation is the file name (without the extension), so in this case it would be:

C1_ports_shipping_aq CDS3_wind_10m_u

@FedericoRondoni
Copy link
Contributor Author

Ah ok! Clear
Thanks Daniel

@santilland
Copy link
Collaborator

Looks good, this should apply for your next commit then

@santilland
Copy link
Collaborator

@FedericoRondoni please make sure that the license field uses a spdx license identifier as defined here:
https://spdx.org/licenses/

@FedericoRondoni
Copy link
Contributor Author

@FedericoRondoni please make sure that the license field uses a spdx license identifier as defined here: https://spdx.org/licenses/

Hi Daniel, I checked already, but I couldn't find any relevant license. I will check again.
However, is there a procedure to bypass this issue if one license is not present in the spdx catalogue?

@santilland
Copy link
Collaborator

If i understand the STAC definition, the value can also be various or proprietary. We could set the license to proprietary and then add the license definition as asset if it exists online somewhere or we save it in the assets repository

santilland and others added 3 commits February 21, 2024 16:08
* fix: invalids yaml indentation; made type optional for service sh endpoint

* chore: fixing DataSource structure, reapplying yaml formatting

* feat: implemented loading of DataSource options

* chore: removed no longer needed code
@FedericoRondoni
Copy link
Contributor Author

Hi @santilland, I also added the license field to two yaml files here, but maybe there is something that you need to implement in this branch as well?

@santilland
Copy link
Collaborator

Yes, we will need to merge the updates from the other branch to this one, i can do it once we merge the other branch

@FedericoRondoni
Copy link
Contributor Author

Yes, we will need to merge the updates from the other branch to this one, i can do it once we merge the other branch

Ok perfect!

@santilland
Copy link
Collaborator

All right, changes integrated, automatic build is passing now again

@FedericoRondoni
Copy link
Contributor Author

Hi guys @santilland @lubojr once the merging of the assets branch YAML-alignment is done, we can ask Anca to review these YAMLs and descriptions.
We will continue to prepare the other batches.

@santilland santilland requested a review from aapopescu March 4, 2024 08:28
@santilland
Copy link
Collaborator

@aapopescu this pull request is the first batch ready to be merged, you can see the preview of the catalog with following url:
https://eodash-testing.eox.at/ui-panels-cat-stacinfo/?catalog=yaml_test

Copy link
Contributor

Choose a reason for hiding this comment

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

where is the Subtitle used?

Copy link
Contributor

Choose a reason for hiding this comment

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

can we group all three indicators into a single one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are current not displaying it in the dashboard, i thought while doing data sanitation it might be an interesting attribute to have if we ever decide to include it

Copy link
Contributor

Choose a reason for hiding this comment

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

cannot find this indicator on the test URL

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems it is not added in the pull request description for it to be generated, will recheck

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, you are right. Now it should be ok

Copy link
Contributor

Choose a reason for hiding this comment

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

rename to "Vessel Density - all types of ships"

Copy link
Contributor

Choose a reason for hiding this comment

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

can we group all EMODNET indicators into a single one?

@santilland
Copy link
Collaborator

@FedericoRondoni @aapopescu the new stac structure was introduced and merged into this branch, we also merged the test branch of the client, so you can explore the client with the current data here:
https://eodash-testing.eox.at/ui-panels-cat/?catalog=yaml_test
I will write an extra mail with information on "grouping".

@lubojr
Copy link
Member

lubojr commented Apr 8, 2024

@FedericoRondoni Please whenever there are renamings of the name of YAML files, these changes need to be reflected in the corresponding catalog YAML, otherwise the build fails. I will take care of fixing this in the already merged PRs batch_3, batch_4 and batch_5 but could you please address the issues in this PR?
For example collections/C2_ports_cars_aq.yaml -> collections/C2_ports_people_aq.yaml has to be changed in https://github.com/eurodatacube/eodash-catalog/blob/main/catalogs/race.yaml

Copy link
Member

@lubojr lubojr left a comment

Choose a reason for hiding this comment

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

adapt RACE.yaml to file renamings

Copy link
Member

@lubojr lubojr left a comment

Choose a reason for hiding this comment

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

Ah nevermind, it is already done in this PR, sorry. It was just the issue in batch_3, batch_4, and batch_5

@lubojr lubojr merged commit f95a0de into main Apr 9, 2024
1 check passed
@lubojr lubojr deleted the yaml_test branch April 9, 2024 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants